Driver for hiwonder encoder motor module #25616
Conversation
chfriedrich98
force-pushed
the
pr-hiwonder_mc_actuators
branch
2 times, most recently
from
840d00a to
e9bbe9a
Compare
chfriedrich98
force-pushed
the
pr-hiwonder_mc_actuators
branch
from
8529548 to
8cabde4
Compare
There was a problem hiding this comment.
Can you structure the driver to match the style of the other I2C drivers? Take a look at the drivers that inherit I2CSPIDriver and try to follow the same patterns. Also the main.cpp is kind of a legacy thing but generally used only to separate out the boiler plate driver functions (main, spawn, init, print_status, etc). For a small driver like this I would keep it to just the HiwonderEMM.cpp/hpp files.
chfriedrich98
force-pushed
the
pr-hiwonder_mc_actuators
branch
from
8cabde4 to
b47e6a8
Compare
chfriedrich98
force-pushed
the
pr-hiwonder_mc_actuators
branch
from
815dca6 to
54c0013
Compare
| static constexpr uint8_t I2CBUS = 1; // I2C bus number | ||
| static constexpr uint8_t CHANNEL_COUNT = 4; // Number of output channels | ||
|
|
||
| class HiwonderEMM : public ModuleBase<HiwonderEMM>, public OutputModuleInterface, public device::I2C |
There was a problem hiding this comment.
Any reason why you're using ModuleBase + device::I2C instead of I2CSPIDriver? The I2CSPIDriver is cleaner and gets you on the right i2c work_queue from the start. It also allows configurable i2c bus/addr.
There was a problem hiding this comment.
I am running into an inheritance issue:
Both I2CSPIDriver and OutputModuleInterface inherit from WorkItem.
WorkItem has a pure virtual function Run() which gets overridden in I2CSPIDriver with the final keyword.
So I cannot override it in my class and OutputModuleInterface seems not to recognize the definition from the I2CSPIDriver (i get a compile error that HiwonderEMM is an abstract class). I suppose this is because none of these classes do virtual inheritance?
I am certainly no expert on this, so if there is a fix for it I would have no problem with using the I2CSPIDriver.
There was a problem hiding this comment.
Okay no worries, probably not worth the effort to fix this. Your implementation looks like it should work just fine
| type: enum | ||
| values: | ||
| 0: Disabled | ||
| 1: Enabled |
There was a problem hiding this comment.
| type: enum | |
| values: | |
| 0: Disabled | |
| 1: Enabled | |
| type: boolean |
|
|
||
| #define DRV_INS_DEVTYPE_SBG 0xEC | ||
|
|
||
| #define DRV_MOTOR_DEVTYPE_HIWONDER_EMM 0xEB |
There was a problem hiding this comment.
| #define DRV_MOTOR_DEVTYPE_HIWONDER_EMM 0xEB | |
| #define DRV_MOTOR_DEVTYPE_HIWONDER_EMM 0xED |
|
|
||
| int HiwonderEMM::set_motor_speed(const uint8_t *speed_values, const uint8_t count) | ||
| { | ||
| uint8_t cmd[1 + CHANNEL_COUNT]; |
There was a problem hiding this comment.
if count < CHANNEL_COUNT, make sure to init the rest of cmd as well
| uint8_t cmd[1 + CHANNEL_COUNT]; | |
| uint8_t cmd[1 + CHANNEL_COUNT] {}; |
| const int ret = transfer(cmd, sizeof(cmd), nullptr, 0); | ||
|
|
||
| if (ret != PX4_OK) { | ||
| PX4_ERR("Failed to set motor speed. Error: %d", ret); |
There was a problem hiding this comment.
This will lead to spamming. Can you make sure to print it only once?
| { | ||
| } | ||
|
|
||
| int HiwonderEMM::init() |
There was a problem hiding this comment.
If I remember correctly, this is fine, but it would be better to call the I2C transfers from the I2C work queue. This is what I2CSPIDriverBase does for you (https://github.com/PX4/PX4-Autopilot/blob/main/platforms/common/i2c_spi_buses.cpp#L644).
New Driver
Add driver for Hiwonder Encoder Motor Driver.
The driver was tested with the Hiwonder Vehicle Chassis (Ackermann, Tracked and Mecanum) which use the motor driver to control the wheel motors.
Logs: