mavlink split BATTERY_STATUS from SYS_STATUS update #10135
Conversation
dagar
force-pushed
the
pr-mavlink_sysstatus
branch
2 times, most recently
from
1b39a77 to
e91d522
Compare
| // if vehicle_status updated always grab latest cpuload and battery_status | ||
|
|
||
| battery_status_s battery_status = {}; | ||
| _battery_status_sub->update(&battery_status); |
There was a problem hiding this comment.
Do you know you will always have valid data for all messages because both battery_status and cpuload are published more frequently than vehicle_status?
If so, aren't you at risk of using old/queued data from battery_status and cpuload?
There was a problem hiding this comment.
In any normal usage we can expect to have them continuously (if we ever had them). Deciding if it's worth handling the failure case is a fair point.
For example if you had a smart battery and lost communication. The now separate BATTERY_STATUS message would stop, but SYS_STATUS battery fields would be stale.
There was a problem hiding this comment.
What I meant was when the system is running as normal and e.g. battery_status publishes at 3x the rate of vehicle_status, the messages would stack up and you'd use an old message.
I don't know how big the stack is but I have seen something to the effect of the following in other places:
while(_battery_status_sub->update(&battery_status)) {}
There was a problem hiding this comment.
That's only an issue for topics that are queued. Currently that's vehicle_command (+ack), subsystem_info, and transponder_report.
- fix SYS_STATUS usage of battery_status and cpuload
|
Actually, I think the real problem here is that the update implementation drifted from it's original design and documentation. https://github.com/PX4/Firmware/blob/master/src/modules/mavlink/mavlink_orb_subscription.h#L56-L65 I'll review. |
dagar
force-pushed
the
pr-mavlink_sysstatus
branch
from
e91d522 to
65246d6
Compare
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Closing as stale. |
SYS_STATUS wasn't necessarily getting fresh data from vehicle_status, battery_status, and cpuload.