Out of the CR(H)IB and into the fire #3
Conversation
…rrorCheckingAndPosition # Conflicts: # CMakeLists.txt # include/dev/ADS8689IPWR.h # src/dev/ADS8689IPWR.cpp # targets/ADS8689IPWR/main.cpp # targets/CMakeLists.txt
Ethan-Caracoglia
requested review from
DiegoLHendrix,
XanBu,
aclowmclaughlin,
mjh9585 and
tmb5932
|
Change the .github/workflows/cmake.yml file from |
aclowmclaughlin
left a comment
aclowmclaughlin
left a comment
There was a problem hiding this comment.
This is really impressive work and the code is honestly really clean. I think 90% of everything I commented was simply just some like comments that could be added.
Only code change of significance is using a struct to contain the CAN message instead of an array.
| * ________________/ \__________________ | ||
| * Brake Error Bit Throttle Error Bit | ||
| */ | ||
| uint8_t data[payloadLength]; |
There was a problem hiding this comment.
If we have this really specific layout, why isn't this a struct?
There was a problem hiding this comment.
And if it's because you want it to be addressable as an array, look into unions.
| // main loop | ||
| while (true) { | ||
| hib.process(); | ||
| uart.printf("Throttle Voltage: %i mV\r\n", hib.throttleVoltage); |
There was a problem hiding this comment.
These should be log statements. I know it doesn't seem like a big deal, but uart prints actually take a decent amount of time to run, and log statements have the advantage of being disable-able by simply compiling without EVT_CORE_LOG_ENABLE.
| @@ -0,0 +1,77 @@ | |||
| #include <HIB.hpp> | |||
There was a problem hiding this comment.
Need a file description. (Especially here, where you have two targets. explaining the situations in which one vs the other should be used is very important.
| @@ -1,29 +1,151 @@ | |||
| /** | |||
There was a problem hiding this comment.
Add a description of the main.
mjh9585
left a comment
mjh9585
left a comment
There was a problem hiding this comment.
Overall pretty good, just a few changes besides the ones mentioned. Please include the Jira Issue ID in the pull request name like the other.
| io::SPI& spiBrake = io::getSPI<io::Pin::PC_10, io::Pin::PC_12, io::Pin::PC_11>(brakeDevices, deviceCount); | ||
| spiBrake.configureSPI(SPI_SPEED, io::SPI::SPIMode::SPI_MODE0, SPI_MSB_FIRST); |
There was a problem hiding this comment.
Add a comment about setting up SPI and the ADCs.
| uint8_t payload[payloadLength]; | ||
| uint32_t throttleVoltage = 0; | ||
| uint32_t brakeVoltage = 0; | ||
|
|
||
| // Counters for throttle errors | ||
| uint64_t acceptableThrottleMarginErrors = 0; | ||
| uint64_t precisionThrottleMarginErrors = 0; | ||
| uint64_t comparisonThrottleErrors = 0; | ||
|
|
||
| // Counters for brake errors | ||
| uint64_t acceptableBrakeMarginErrors = 0; | ||
| uint64_t precisionBrakeMarginErrors = 0; | ||
| uint64_t comparisonBrakeErrors = 0; |
There was a problem hiding this comment.
These variables should be private unless there is a good reason for public access. And if so add a comment noting why.
| payload[0] = static_cast<uint8_t>(throttleVoltage >> 8 & 0xFF); | ||
| payload[1] = static_cast<uint8_t>(throttleVoltage & 0xFF); | ||
| payload[2] = static_cast<uint8_t>(brakeVoltage >> 8 & 0xFF); | ||
| payload[3] = static_cast<uint8_t>(brakeVoltage & 0xFF); |
There was a problem hiding this comment.
This would probably be better as a struct and if the bytes are in the wrong order you could use __rev or __rev16 to rearrange the bytes. https://developer.arm.com/documentation/dui0491/i/Compiler-specific-Features/--rev-intrinsic
Co-authored-by: Diego <[email protected]> Co-authored-by: Rue <[email protected]> Co-authored-by: Matthew Heller <[email protected]>
Created Drivers for the ADS8689IPWR ADC sensors that get processed through SPI and packaged into a 5 byte CAN message that gets sent to the VCU giving throttle, brake, and error information.