Fix incorrect sign-magnitude handling for STS/SMS series encodings #2223
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this does
We noticed that in our sm8512bl servos that after moving below zero with torque disabled we get a jump to over ~327xx. After some debugging we noticed that the integer that is sent is signed, which is documented in the driver code of the feetech python library: https://gitee.com/ftservo/FTServo_Python/blob/main/scservo_sdk/sms_sts.py
This pull request fixes the incorrect sign-magnitude handling for STS/SMS series encodings so values don’t “wrap” to ~32k when crossing zero. Specifically, we update the encoding bit mapping used for sign-magnitude fields:
This was also noticed by @MoffKalast for the STS3215s servos on the lerobot discord (video is from them): https://discord.com/channels/1216765309076115607/1237741463832363039/1427617779267403897
wat.mp4
Why this matters: on SM8512BL and STS3215 we observed the present position flipping to >32768 after going below 0.
Using the proper sign-magnitude bit fixes the wrap.
References:
position = scs_toscs(position, 15)and readsscs_tohost(..., 15)for position and speed, i.e., sign bit at 15. Same for wheel speed. ([gitee.com][1])How it was tested
Present_Position.How to checkout & try? (for the reviewer)
You should see no jump to ~327xx when you cross negative to positive.
I've used the above script to test this on our sm8512bl and the issue is gone.
Notes and open questions
Normalization expectations. Some users expect angles in
[0, MODEL_RESOLUTION)rather than signed host units. After this fix, values are decoded correctly into signed host units. If your public API promises[0..4095](or model-specific resolution),_normalizeshould map signed values with:and document whether positions are returned as signed ticks vs modulo resolution
Feel free to rewrite this, if you have a better solution that fits the library better, but I've wanted to document this somewhere and I think this addition doesn't make things worse. :)