Skip to content

Processor 2025 Full Review #90

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

Merged
merged 27 commits into from
Nov 26, 2024
Merged

Processor 2025 Full Review #90

merged 27 commits into from
Nov 26, 2024

Conversation

Armaan-Sengupta
Copy link
Contributor

No description provided.

Added Movella IMU UART 8 connection and fixed a bunch of DRC errors
Updated kicad footprints and symbols to match new rebased structure
Moved my two vendor libraries within the project now rather than to my local disk
@Joe-Joe-Joe-Joe
Copy link
Contributor

Joe-Joe-Joe-Joe commented Nov 2, 2024

  1. Why is this called "spi_addr"? Shouldn't it be "I2C_addr" or smth?
    image
  2. For net labels beside hierarchical ones they are not technically overlapping but still very hard to read. Please extend one as has been done for CAN Tx/Rx above
    image
  3. This is my failure from before but this should live in the whitespace on the right side and have a net label to PH0
    image
  4. Please point power symbols up and grounds down always
    image
  5. What....
    image
  6. Is this fuse value on 5V still accurate? Same with R1. Have you actually done your power draw calcs somewhere? If you need a reference, "processor board current"
    image

@Joe-Joe-Joe-Joe
Copy link
Contributor

  1. When you do get to layout, make sure you annotate the axis directions of the LSM clearly on the silkscreen. Also note that their "X" and "roll" axes are different, which is not the same as our (well, my, but nonetheless the correct one) axis convention for the rocket
  2. What does Movella Sync In/Sync Out do? Does it make sense to connect it to timer pins (I think you have those free somewhere)
  3. D:
    image
  4. Inductor packages tend to be nonstandard. Have you selected a specific inductor for L1? There is a datasheet section on inductor selection which you should read if you haven't already.

@StarlightDescender
Copy link
Contributor

  1. Pin 6 is a NC pin for the 2568? If so, no point in including it in the symbol.
  2. Max current on the buck is 1A - are you sure this is sufficient? Have you double checked your current draw maximums? It seems pretty close from my review of the controls 2024 spreadsheet and the changes made, but I'd double check.
  3. nit but the datasheet on U5 links to the page on stm, rather than the datasheet pdf
  4. Not super familliar with I2C but what's the justification for the DNP resistors R16/17/19/21?
  5. also nit but there's some inconsistent formatting with resistor values - there are 10k resistors marked as 10kΩ or 10kR. afaik team standard is 10k, same with capacitors (1u vs 1uF)
  6. Bold to assume you can find an inductor with a standard footprint - take a look on digikey for the right value and see what you find, pick something and use it's footprint. We ran into issues with finding standard footprint inductors with charging board last year, and I suspect you'll find the same this year.

@Armaan-Sengupta
Copy link
Contributor Author

@Joe-Joe-Joe-Joe :

  1. Yes sry, too much SPI at work
  2. ty
  3. yea good idea, didnt notice that was the only connection lol
  4. 👍
  5. test that you are really getting 3.3v before it powers the entire system up potentially breaking everything if it is not 3.3v (ex. wrong resistors for voltage divider), then just put a blob of solder there to connect
  6. I have not, just carried over, thanks for catching that. But upon checking it should be fine, buck allows current to be a bit lower, but that should be fine.
  7. Good idea, I will follow the sensor's coordinate frame rather than our team standard
  8. I dont think they will be required, they are there primarily because harwin is 6 pin, lol. I think we are most likely to use the Sync In if we want to specifcally request data at that instant. Yea I can tie sync in to a clock line, I still think we will probably use it as a regular GPIO but doesnt hurt.
  9. Fixed
  10. I did, but I would like to read that resource to check against, can you send it?

@CasualIntellectual :

  1. Its a vendor part, id rather just leave it to match the datasheet, and so we dont need to modify the provided file
  2. Yea, it will be less than 1A, calcs linked above
  3. Fixed
  4. We can place them if we want, resistors in parallel lower the equivalent resistance (weaker pullup), RC time constant goes down, we can run I2C faster if we want
  5. Fair enoug for resistors, just thought it looked nicer, but removed👍 . For caps it looks really strange IMO, but if its fr a team standard we follow I can remove it (same for inductor ig).
  6. I did check, and just oversized, I think its too oversized, ill learn how to make a custom footprint during routing

@Joe-Joe-Joe-Joe
Copy link
Contributor

Joe-Joe-Joe-Joe commented Nov 6, 2024

I did, but I would like to read that resource to check against, can you send it?

You want me to send you the datasheet for the buck converter?

I did check, and just oversized, I think its too oversized, ill learn how to make a custom footprint during routing

Please don't do this unless you cannot find a footprint for it online.

@Armaan-Sengupta
Copy link
Contributor Author

Armaan-Sengupta commented Nov 6, 2024

10. datasheet section on inductor selection

Oh I assumed this would not live in a specific Inductor's datasheet, but rather be some kind of general third party guide. If not nw.

Next step is to start PCB routing
Moved the harwins into the right place, still need to route those, got rid of board outline, and moved a bunch of components off the pcb to reorganize
NEED TO CHECK
DRC errors still need to be solved
More DRC issues to fix
@StarlightDescender
Copy link
Contributor

  1. This via is only connected on one side, I think you can just remove it?
    image

  2. aaaaaahhhhhh what is this daisychaining
    image

  3. Is there a reason why all of your test points are through-hole?

  4. There's a lot of pad-silkscreen overlap in general

  5. Test points not labeled?

@Armaan-Sengupta
Copy link
Contributor Author

  1. Good catch
  2. Good point ty
  3. Not all, the I2C ones are not, where I had space to fit them, I made them through hole since its more convenient
  4. Yup hadnt gotten to cleaning that up yet, you got the jump on me before I PR'd this lol (but ty I appreciate the early feedback)
  5. Which ones? Most are looks like I missed the 5v and gnd inputs, still need to work on silkscreen stuff

Connected unconnected SCL test point
Removed redundant via
Removed daisy chained ground
@Armaan-Sengupta Armaan-Sengupta self-assigned this Nov 12, 2024
2024-11-11 silk screen updates
Minor DRC warnings adressed, and silk screen prettied up
@Armaan-Sengupta Armaan-Sengupta marked this pull request as ready for review November 13, 2024 02:56
@Armaan-Sengupta
Copy link
Contributor Author

Harwin connections flipped to other side of bord as per Jason request, and pin connections re-done as per Joe's comment. change log of harness connections here.

Some cosmetic changes now that we know the board will be mounted flipped.

Final step before production is for me to source matching components on JLCPCB and setup all the PCBA files correctly (BOM + CPL) but in terms of the PCB itself, it is done, please if you have reviewed it, mark it as approved so we can merge, else kindly review it first. Thanks for the help yall.

Copy link

@Redstone-ray Redstone-ray left a comment

Choose a reason for hiding this comment

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

  1. What is this, where is this going
    image
  2. b b b b b b b b b
    image
  3. Where is this x pin going
    image
  4. woah 90 degrees
    image

@Joe-Joe-Joe-Joe
Copy link
Contributor

So, I still have some reservations about this. I understand flipping the rocketCAN connector to mate with the new sled. But if you are flipping the other connectors does that mean they will also mate with the sled, and you will plug the external IMUs into the sled as well? Are you going to try to connect them directly and you just need to meet the clearance requirement? I am very lost as to how this is coming together.

@Joe-Joe-Joe-Joe
Copy link
Contributor

Also fwiw I won't approve it until you push the final production files

@Joe-Joe-Joe-Joe
Copy link
Contributor

@Redstone-ray from the electrical standard:
image
In this case they are all GPIOs and it doesn't matter either way but tying NC pins to ground is absolutely not good practice

@Armaan-Sengupta
Copy link
Contributor Author

@Redstone-ray from the electrical standard:

For future reference. Joe's comment is regarding the standard practise for NC pins is for motor controller board re: richards comment. It is not related to processor.

@Armaan-Sengupta
Copy link
Contributor Author

So, I still have some reservations about this. I understand flipping the rocketCAN connector to mate with the new sled. But if you are flipping the other connectors does that mean they will also mate with the sled, and you will plug the external IMUs into the sled as well? Are you going to try to connect them directly and you just need to meet the clearance requirement? I am very lost as to how this is coming together.

@JasonBrave idk about how recelec looks, you told me constraints for mounting I simply followed, if you could answer Joes questions that would be great

When flipping the harwin to the back side of the board it accidentally overlapped with the SD card so moved and re-routed it. Also all the libraries both footprint and schematic went missing so relinked all.
Sourced similar parts for all chosen components for JLCPCB to assemble onto the board, linked these as a part property for each symbol (updating common symbol library for resistors and capacitors to include this by default). Also updated which components (harwins, SD card, and test points dont need to be in the BOM)
Redstone-ray
Redstone-ray previously approved these changes Nov 22, 2024
@Redstone-ray
Copy link

Redstone-ray commented Nov 22, 2024

Please confirm manufacturing details and ST IMU can be assembled before ordering, and update title block for your layout & schematic

@rexblade21
Copy link

image
Why is this routed out and then connected to the 5V pour with vias as opposed to connecting it directly to the 5V pour on the top layer?

added names to sheets + minor reroute of 5V cause y not
rexblade21
rexblade21 previously approved these changes Nov 23, 2024
@Redstone-ray Redstone-ray self-requested a review November 23, 2024 06:22
Redstone-ray
Redstone-ray previously approved these changes Nov 23, 2024
Copy link

@Redstone-ray Redstone-ray left a comment

Choose a reason for hiding this comment

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

Idk why you just copper pour this or run a trace out instead of via it from power but sure can't give two less cents of fucks
image

@Joe-Joe-Joe-Joe
Copy link
Contributor

Joe-Joe-Joe-Joe commented Nov 23, 2024

You need to regenerate the drill files (make sure they replace the ones in gerbers/, the default puts them in production/)

@Joe-Joe-Joe-Joe
Copy link
Contributor

JLC has a specific format for gerbers/drill files, make sure you follow it

https://jlcpcb.com/help/article/how-to-generate-gerber-and-drill-files-in-kicad-8

@Armaan-Sengupta
Copy link
Contributor Author

Deleted those gerber files they were stale and from the old way of manually doing it, all relevant production files are in production folder, and all gerbers are in the zip file which you can directly upload to JLCPCB.

Copy link
Contributor

@StarlightDescender StarlightDescender left a comment

Choose a reason for hiding this comment

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

LGTM, good job 👍 (review altimeter please 🥺)

@Armaan-Sengupta Armaan-Sengupta merged commit 4d821bd into master Nov 26, 2024
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.

6 participants