AP_DroneCAN: stop reversing ARDUPILOT_INDICATION_NOTIFYSTATE_VEHICLE_… #30831
Conversation
|
|
||
| msg.aux_data_type = ARDUPILOT_INDICATION_NOTIFYSTATE_VEHICLE_YAW_EARTH_CENTIDEGREES; | ||
| uint16_t yaw_cd = (uint16_t)(360.0f - AP::ahrs().get_yaw_deg())*100.0f; | ||
| const uint16_t yaw_cd = (uint16_t)(AP::ahrs().get_yaw_deg()*100.0f); |
There was a problem hiding this comment.
This looks like a good change, but does it deserve a wrap_360cd()? I only ask because there seems to be some 360 maths that you're replacing here
There was a problem hiding this comment.
This looks like a good change, but does it deserve a wrap_360cd()? I only ask because there seems to be some 360 maths that you're replacing here
Thankfully get_yaw_deg comes pre-wrapped (update_cd_values). I'm holding onto and using that fact :-)
There was a problem hiding this comment.
@peterbarker The reason it was done this way to support how it was implemented originally in HerePro release firmware before AP_Periph became stable. Is it possible to maybe rename the constant to something like VEHICLE_YAW_HEREPRO_LED, changing constant names don't change the signature of DSDL message so it should be fine, and we can remove ambiguity from what this field actually does.
|
needs review by @bugobliterator |
|
|
||
| msg.aux_data_type = ARDUPILOT_INDICATION_NOTIFYSTATE_VEHICLE_YAW_EARTH_CENTIDEGREES; | ||
| uint16_t yaw_cd = (uint16_t)(360.0f - AP::ahrs().get_yaw_deg())*100.0f; | ||
| const uint16_t yaw_cd = (uint16_t)(AP::ahrs().get_yaw_deg()*100.0f); |
There was a problem hiding this comment.
@peterbarker The reason it was done this way to support how it was implemented originally in HerePro release firmware before AP_Periph became stable. Is it possible to maybe rename the constant to something like VEHICLE_YAW_HEREPRO_LED, changing constant names don't change the signature of DSDL message so it should be fine, and we can remove ambiguity from what this field actually does.
tridge
left a comment
tridge
left a comment
There was a problem hiding this comment.
unfortunately I think the best option is just add comments
|
|
||
| msg.aux_data_type = ARDUPILOT_INDICATION_NOTIFYSTATE_VEHICLE_YAW_EARTH_CENTIDEGREES; | ||
| uint16_t yaw_cd = (uint16_t)(360.0f - AP::ahrs().get_yaw_deg())*100.0f; | ||
| const uint16_t yaw_cd = (uint16_t)(AP::ahrs().get_yaw_deg()*100.0f); |
There was a problem hiding this comment.
I think given the answer from @bugobliterator we should add a comment in the code, plus add a comment in the DSDL file explaining the orientation convention
peterbarker
force-pushed
the
pr/yaw-360-reversal-periph
branch
from
5a79d54 to
76d6506
Compare
|
Changed to just comment the existing behaviour |
|
DroneCAN PR is here: dronecan/DSDL#72 |
6 participants
…YAW_EARTH_CENTIDEGREES
this number is already wrapped. This reverses the yaw direction so if the vehicle was pointing 15 degrees East then this would change it to be 15 degrees West...
I believe this was created so a HereGPS could use its blinkenlights to indicate where the vehicle thought North might be.
There are currently no in-tree users of the LUA binding which was probably used to get the effect.
Can we change this?