Motor driver firmware integration #3458
Conversation
…ts_crosstool_compiler
…re into tbots_crosstool_compiler
…e/Software into motor_firmware_integration
…into motor_firmware_integration
williamckha
force-pushed
the
motor_firmware_integration
branch
from
c369820 to
0982f95
Compare
| NO_FAULT = 15; | ||
| DURATION = 16; | ||
| OVER_VOLT = 17; | ||
| UNDER_VOLT = 18; | ||
| OVER_TEMP = 19; | ||
| START_UP = 20; | ||
| SPEED_FDBK = 21; | ||
| OVER_CURR = 22; | ||
| SW_ERROR = 23; | ||
| SAMPLE_FAULT = 24; | ||
| OVERCURR_SW = 25; | ||
| DP_FAULT = 26; |
There was a problem hiding this comment.
since we have control over the faults now, I'd suggest naming these to be more explicative of the fault
| SPI_ERROR = 0b11100000 | ||
| }; | ||
|
|
||
| enum class StSpinFaultCode |
There was a problem hiding this comment.
instead of redefining the enum, can we re-use the proto definitions?
There was a problem hiding this comment.
I guess the intention is to map it to a bit mask correct?
|
|
||
| MotorFaultIndicator StSpinMotorController::checkDriverFault(const MotorIndex& motor) | ||
| { | ||
| bool drive_enabled = true; |
There was a problem hiding this comment.
I see that you've copied over the trinamic faults. we should only have these faults if we are reporting them in the firmware (iirc we don't rn)
| const int16_t speed = sendAndReceiveFrame(motor, StSpinOpcode::GET_SPEED, 0); | ||
|
|
||
| // SET_SPEEDRAMP expects the target motor speed to be in register ax. | ||
| sendAndReceiveFrame(motor, StSpinOpcode::MOV_AX, | ||
| static_cast<int16_t>(target_velocity)); | ||
|
|
||
| // SET_SPEEDRAMP expects the ramp time in millis to be in register bx. | ||
| // We do speed ramping ourselves in MotorService, so we just want to | ||
| // set the target speed without ramping (hence we set reg bx to 0). | ||
| sendAndReceiveFrame(motor, StSpinOpcode::MOV_BX, 0); | ||
|
|
||
| sendFrame(motor, StSpinOpcode::SET_SPEEDRAMP); |
There was a problem hiding this comment.
it looks like we're doing 7 SPI transactions to read/write the speed for a single motor. SPI transactions are costly (because there's a kernel context switch and memory copies involved). We should be trying to do this in max 2 transactions. Suggest reworking the firmware API here
| tx[1] = static_cast<uint8_t>(opcode); | ||
| tx[2] = static_cast<uint8_t>(0xFF & (data >> 8)); | ||
| tx[3] = static_cast<uint8_t>(0xFF & data); | ||
| tx[4] = 0; // CRC; to be implemented later |
| uint8_t tx[FRAME_MAX_LEN] = {0}; | ||
| uint8_t rx[FRAME_MAX_LEN] = {0}; | ||
|
|
||
| tx[0] = FRAME_SOF; |
There was a problem hiding this comment.
might be worth having a few bits for versioning (so that incompatible motor FW and software FW don't interact)
|
|
||
| tx[0] = FRAME_SOF; | ||
| tx[1] = static_cast<uint8_t>(opcode); | ||
| tx[2] = 0; // CRC; to be implemented later |
| #include "software/embedded/motor_controller/motor_fault_indicator.h" | ||
| #include "software/embedded/motor_controller/motor_index.h" | ||
|
|
||
| class MotorController |
| #include "software/embedded/motor_controller/motor_index.h" | ||
| #include "software/embedded/motor_controller/stspin_constants.h" | ||
|
|
||
| class StSpinMotorController : public MotorController |
| #include "software/embedded/motor_controller/motor_fault_indicator.h" | ||
| #include "software/embedded/motor_controller/motor_index.h" | ||
|
|
||
| class TmcMotorController : public MotorController |
| * Leaves the motors connected in MOTION_MODE_VELOCITY | ||
| */ | ||
| void checkEncoderConnections(); | ||
| std::unique_ptr<MotorController> setupMotorController(); |
Please fill out the following before requesting review on this PR
Description
Testing Done
Resolved Issues
Length Justification and Key Files to Review
Review Checklist
It is the reviewers responsibility to also make sure every item here has been covered
.hfile) should have a javadoc style comment at the start of them. For examples, see the functions defined inthunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.TODO(or similar) statements should either be completed or associated with a github issue