IridiumSBD Debug Message + Acknowlege fix #9326
Conversation
|
Looks good. The check if you need to publish is a bit verbose. That makes me wonder if something similar could be pushed into the uORB layer itself (that already has the last message). |
|
Do you want to add iridiumsbd_status to logger? |
|
I added it to the logger. I guess this check is quite specific to the status messages. In general I think the messages would almost always change when published, for example sensor data, control inputs, estimator data, ... |
src/modules/commander/commander.cpp
Outdated
| @@ -1056,7 +1056,10 @@ Commander::handle_command(vehicle_status_s *status_local, | |||
| break; | |||
| case vehicle_command_s::VEHICLE_CMD_CONTROL_HIGH_LATENCY: { | |||
| // only send the acknowledge from the commander, the command actually is handled by each mavlink instance | |||
| cmd_result = vehicle_command_s::VEHICLE_CMD_RESULT_ACCEPTED; | |||
| // only send the acknowledge if the command is received from an external source | |||
There was a problem hiding this comment.
Why isn't VEHICLE_CMD_CONTROL_HIGH_LATENCY handled entirely by the mavlink module?
There was a problem hiding this comment.
The acknowledge is handled by the commander. The reason is that in that way transmitting multiple acknowledges is avoided.
There was a problem hiding this comment.
Sure, but you also lose the ability to even potentially acknowledge in a meaningful way.
Even if you only sent the acknowledge in the mavlink task that's in iridium mode at least you'd get no response when Iridium is disabled instead of a meaningless acceptance.
There was a problem hiding this comment.
Which is also not ideal because then QGC will be resending the command multiple times. What I could do here is to check if any high_latency telemetry exists and decide based on that if the command is accepted or denied. Would that be acceptable?
The missing piece here would be that it is not checked if a correct mavlink instance exists. But if you check it on mavlink you can also not check if any iridium driver is started so it is also not completely safe.
There was a problem hiding this comment.
"QGC will be resending the command multiple times"
Why if only the Iridium mavlink instance is responding at all? https://github.com/PX4/Firmware/blob/master/src/modules/mavlink/mavlink_main.cpp#L2204-L2220
Checking across all mavlink instances (so you could send the denied) would be even better of course.
There was a problem hiding this comment.
This should be moved to the MAVLink module and acknowledged there. In terms of retransmission: If Iridium is not active, it should send a NACK and that will tell QGC that it is not supported.
|
Sounds like it would be cleaner to do the ack in MAVLink. |
src/modules/commander/commander.cpp
Outdated
| @@ -1056,7 +1056,10 @@ Commander::handle_command(vehicle_status_s *status_local, | |||
| break; | |||
| case vehicle_command_s::VEHICLE_CMD_CONTROL_HIGH_LATENCY: { | |||
| // only send the acknowledge from the commander, the command actually is handled by each mavlink instance | |||
| cmd_result = vehicle_command_s::VEHICLE_CMD_RESULT_ACCEPTED; | |||
| // only send the acknowledge if the command is received from an external source | |||
There was a problem hiding this comment.
This should be moved to the MAVLink module and acknowledged there. In terms of retransmission: If Iridium is not active, it should send a NACK and that will tell QGC that it is not supported.
acfloria
force-pushed
the
feature/iridium_debug_message
branch
from
5f48d75
to
e0ddaae
Compare
|
@LorenzMeier @dagar can you look at the changes I made according to your comments. |
|
Tested? |
|
Yes |
…from_external is false
- Move publishing the telemetry status from the IridiumSBD driver to the mavlink instance - In the commander use the iridiumsbd_status message for heartbeat in case of a high latency link - Move positive acknowledge to the mavlink instance - Add a failed acknowledge in the commander if no high latency link exists
acfloria
force-pushed
the
feature/iridium_debug_message
branch
from
e0ddaae
to
b6438ca
Compare
|
Rebased and added a fix which avoids a rare case where the driver was stuck in a state after it received data from the ground station. @LorenzMeier your feedback on the recent changes would be appreciated. |
- Catch the case where the case where the driver gets stuck because nothing is received by +SBDRB - Add a mutex for the rx buffer - Stop the standby loop if a mode change is already scheduled
acfloria
force-pushed
the
feature/iridium_debug_message
branch
from
b6438ca
to
41bcef1
Compare
|
@LorenzMeier @dagar status? |
|
This looks good for now. In the near future I'd like to take another pass and cleanly split telemetry_status. |
Add a message with the status of the iridiumsbd driver. It is published at every loop of the driver if the status changed.
Only send the acknowledge for the VEHICLE_CMD_CONTROL_HIGH_LATENCY command if the command is coming from external.