Pointers with implementing a new encoder mode - Hall plus absolute encoder

Hello there,

Firstly thank you so much for making an awesome BLDC driver AND opensouring it! :smiley:

I have a motor with a 100:1 wormgear attached.
Up until this point I have been making do with position control using just the hall effect, which has been just about good enough for my low precision application due to the high ratio gearbox, but I would like to make it a bit more precise now plus home to a preset position on boot.

So I’m attempting to modify the firmware (main branch) to allow position control using the motors in built hall effect but also with an absolute encoder on the gearbox output shaft to feedback the true position to pos_estimate. If this is not possible or I should be going about this in a different way then please let me know.

I have a AMT232B working and can see its position in odrivetool as axis0.encoder.pos_abs.
I added a new encoder mode (260) and spliced the hall effect control into it, plus made it so that only pos_abs was updated by the AMT232. The other control variables are still updated by hall effect only.
All seems fine (both hall and AMT232 positions being updated) until I try to move to a new position which results in:

Seems there is a conflict in the timing for the two modes combined? But I get a bit lost on trying to separate them out and I found that they both have comments saying that they probably shouldn’t be being updated where they currently are.
In low_level.cpp:

417 - // TODO: this is out of place here. However when moving it somewhere
// else we have to consider the timing requirements to prevent the SPI
// transfers of axis0 and axis1 from conflicting.
// Also see comment on sync_timers.
if((current_meas_not_DC_CAL && !axis_num) ||
(axis_num && !current_meas_not_DC_CAL)){

467 - // Prepare hall readings
// TODO move this to inside encoder update function

Sorry for the essay!!
Any suggestions much appreciated.

There are many threads about doing similar things with 2 encoders, but none I can see with hall effect and encoder.

I’m not exactly sure what you’ve done here, but take a look at the controller_.config.load_encoder_axis.

Let’s say you are using Axis 0. If you set load_encoder_axis = 1, then Axis 0 would still use Axis0 hall effect sensors for commutation, but use Axis 1’s encoder for position and velocity feedback. What we don’t support (yet) that you could add, is a 2nd encoder for each axis. I know @Samuel plans to do an encoder class refactoring soon and this is one of the features we want to add. Perhaps he can can give you pointers for the current development firmware

@Wetmelon Thanks for that. I can now see I was going about the firmware changes all wrong.

Just tried to use load_encoder_axis but no luck so far.
FW 5.1
Hall effect position control working on axis 0.
Absolute encoder reading correctly into axis 1.
Set odrv0.axis0.controller.config.load_encoder_axis = 1
Save, reboot.
On boot I now have error on axis0 - CONTROLLER_ERROR_INVALID_ESTIMATE
Error cannot be cleared. Am digging into this now but can’t see anything obvious.

I noticed that in the original commit for dual-encoder support that there was a scaling factor - load_encoder_ratio

Is this no longer required or has it been renamed? I can’t see anything similar in effect in the latest code.

What firmware version is giving you the ERROR_INVALID_ESTIMATE? devel branch? or the official release?

Official release 5.1.
Also devel forked not long after the 5.1 release.