Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed silkscreen for J18 #427

Merged
merged 2 commits into from
Nov 3, 2023
Merged

Fixed silkscreen for J18 #427

merged 2 commits into from
Nov 3, 2023

Conversation

abust005
Copy link
Collaborator

Silkscreen for J18 (RC3 Motor Supply) was inconsistent with the net going to those pins

@ericjunkins
Copy link
Collaborator

Can I ask what the scope of this MR is? The title is fixing silk screen and has a commit for that which @apollokit was to review (I think he was out of town, maybe still is). But now I see the WireViz diagrams being added too. I think it would be cleaner to keep those for a separate MR and to do just the silkscreen update here.

@ericjunkins
Copy link
Collaborator

I'm happy to do the review of the silkscreen instead and could do that soon to get this MR in though

@abust005
Copy link
Collaborator Author

Thank you for bringing this to my attention, the WireViz diagrams should not have ended up in this MR.

@ericjunkins
Copy link
Collaborator

oh, these changes are made with a newer version of KiCAD than we were previously using. I think in general I have opposition to upgrading KiCAD version. @abust005 you didn't have any import issues in moving forward in version?

@abust005
Copy link
Collaborator Author

abust005 commented Nov 3, 2023

@ericjunkins I did notice it was a newer version because KiCAD told me, but I was unsure as to what the version currently in use is.

As far as import issues, I'm not aware of any; I didn't run the DRC on anything, but I was able to get the boards fabbed with no issues

@ericjunkins
Copy link
Collaborator

We noticed we were a bit behind on KiCAD versions when we started designing this rev but decided not to upgrade in case we had compatibility issues. But It seems if there isn't an issue then I see no reason not to.

@abust005
Copy link
Collaborator Author

abust005 commented Nov 3, 2023

I'm far from an expert on the matter, so I can't speak very much to any compatibility issues or lack thereof, I can only offer my own anecdotal experience :)

@ericjunkins
Copy link
Collaborator

Looks good to me, here's a quick screencap for everyone else of the component in question and the silkscreen corrected.
image

I have another request looking at this, can you increment the silkscreen pcb version as well? This would be v2.0.3 and has a silkscreen label on both of the boards that need updated.

@abust005
Copy link
Collaborator Author

abust005 commented Nov 3, 2023

Updated version silkscreen per request

Copy link
Collaborator

@ericjunkins ericjunkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved!

@Achllle
Copy link
Collaborator

Achllle commented Nov 3, 2023

@sethfischer Noticing that the readthedocs build is failing due to

Problem in your project's configuration. No default configuration file found at repository's root.

I will merge this in since it's entirely unrelated, but wanted to check if you knew what may be causing this.

@Achllle Achllle merged commit fe69070 into nasa-jpl:master Nov 3, 2023
1 of 2 checks passed
@ericjunkins
Copy link
Collaborator

Thanks for the fix @abust005 !

@sethfischer
Copy link
Contributor

@sethfischer Noticing that the readthedocs build is failing due to

Problem in your project's configuration. No default configuration file found at repository's root.

Read the Docs failed to build this branch because the branch was created off master before #430 was merged.

A git fetch upstream and git rebase upstream master would have allowed the docs to build for this branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants