AP_Periph: add support for extended ESC DroneCAN message. #27772
Conversation
IamPete1
force-pushed
the
periph_extended_esc
branch
from
c3840bf
to
6d25a8f
Compare
IamPete1
force-pushed
the
periph_extended_esc
branch
from
2c4bb11
to
b93b403
Compare
|
I think we can remove the volatiles, then merge on CI pass |
IamPete1
force-pushed
the
periph_extended_esc
branch
from
b93b403
to
73646b0
Compare
I have done some more reading, i think its correct here. It enforces that it must be called on a object which is marked volatile. So because the objects are declared volatile here: The volatile on the method enforces that, so it should be a compiler error if you were to try calling it on a none volatile instance of the object. |
IamPete1
force-pushed
the
periph_extended_esc
branch
2 times, most recently
from
90e4067
to
b95fc49
Compare
| uint32_t last_esc_telem_update_ms; | ||
| void esc_telem_update(); | ||
| uint32_t esc_telem_update_period_ms; | ||
| #if AP_EXTENDED_ESC_TELEM_ENABLED | ||
| void esc_telem_extended_update(const uint32_t &now_ms); |
There was a problem hiding this comment.
Why is this a reference? It isn't written in the function.
There was a problem hiding this comment.
I guess it really comes out the same in this case, but passing the reference is often less data than passing the object.
There was a problem hiding this comment.
Not using the reference will save a bit of flash though.
| uint8_t power_rating_pct; | ||
| if (esc_telem.get_power_percentage(i, power_rating_pct)) { | ||
| pkt.power_rating_pct = power_rating_pct; | ||
| } |
There was a problem hiding this comment.
Could avoid the temporary, the data type should be compatible.
There was a problem hiding this comment.
Yeah, but then you need the whole IGNORE_RETURN thing for the bool. I think this is clearer.
There was a problem hiding this comment.
That should only apply if the function is marked to warn if the return is ignored, which it doesn't seem to be. I think it is clearer to just call the function, perhaps with a comment that all packet fields have been inited to 0. The DSDL does not specify what the "unavailable" value is for this field (though we were sending 0 before).
There was a problem hiding this comment.
It really should be marked to warn..... but I didn't want to get into that in this PR.
IamPete1
force-pushed
the
periph_extended_esc
branch
from
b95fc49
to
4cb080a
Compare
This builds on #27771 to allow testing of both it and #27755.
They do work:
On Periph this is not enabled by default. If it is enabled it sends each ESC in turn such that they are each sent at the set
ESC_EXT_TLM_RATE, by default this is 10 times lower than the default ESC status message rate.I had to add getters into AP_ESC_Telem for periph to read the values. I also added some token values to the SITL esc backend.
I have also added a helper to ESC telem for the get functions to check the type bitmask and if the data is stale in one call.