Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RADIO_STATUS message filtering on Solo #470

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

kellyschrock
Copy link
Contributor

Solo evidently doesn't set msgid on the RADIO_STATUS
message to the correct value, causing those messages
to be ignored. This handles that message specifically
and ignores sysid for the Solo.

Also, added a break for HEARTBEAT_FIRST messages
in the switch in notifyDroneEvent() to avoid getting
2 connection events when first connecting.

Solo evidently doesn't set msgid on the RADIO_STATUS
message to the correct value, causing those messages
to be ignored. This handles that message specifically
and ignores sysid for the Solo.

Also, added a break for HEARTBEAT_FIRST messages
in the switch in notifyDroneEvent() to avoid getting
2 connection events when first connecting.
@@ -172,6 +173,26 @@ public SoloComp getSoloComp() {
return soloComp;
}

@Override
public void onMavLinkMessageReceived(MAVLinkMessage message) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message id is correct, it's the system id and component id are set to a fixed value.

The fix is https://github.com/dronekit/dronekit-android/blob/develop/ClientLib/src/main/java/org/droidplanner/services/android/impl/core/drone/autopilot/apm/ArduPilot.java#L388

and

https://github.com/dronekit/dronekit-android/blob/develop/ClientLib/src/main/java/org/droidplanner/services/android/impl/core/drone/autopilot/generic/GenericMavLinkDrone.java#L581

The change here actually skips to message ID verification to allow the SiK message to be processed. Not sure why that isn't working on Solo. It work for ArduCopter/ArduPlane etc

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Later: You may need to look at this code for why the message is filtered https://github.com/dronekit/dronekit-android/blob/develop/ClientLib/src/main/java/org/droidplanner/services/android/impl/core/drone/autopilot/apm/ArduPilot.java#L394

DroneKit has way to many extends of classes, it gets really confusing at time 😜

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't fix the filtering for Solo. Solo gets msg_radio_status, not msg_radio. It's filtering the message because it's not included as a Mavlink exception message.

Copy link
Contributor Author

@kellyschrock kellyschrock Feb 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this to work with Solo, need to add a separate case for MSG_RADIO_STATUS in ArduPilot (if you're disinclined to have the processing for that message in ArduSolo like I have it now), and implement the filtering exception in ArduSolo.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be expecting RADIO_STATUS should be handled in GenericMavLinkDrone (not ArduPilot) as its a common MAVLink message https://github.com/dronekit/dronekit-android/blob/develop/ClientLib/src/main/java/org/droidplanner/services/android/impl/core/drone/autopilot/generic/GenericMavLinkDrone.java#L601

Either solution would work, but I think when I added the exception method the idea was that is should be used to override extra messages that you need to let through. (You may need to use processRadio... method to normalize the values)

The code for filter messages was adding as part of the plan to allow sys_id other than 1 to work.. That's the next step at least.

@@ -257,7 +278,11 @@ public void run() {
@Override
public void notifyDroneEvent(final DroneInterfaces.DroneEventsType event) {
switch (event) {
case HEARTBEAT_FIRST:
case HEARTBEAT_FIRST: {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't have a onMessage received overloaded in this object, does the 'double connect message' problem go away?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out it does. I'll also investigate why I'm getting double disconnect messages as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the problem goes deeper than that. Whatever changes happened here cause the Solo to not know it's connected. I'm connected, getting radio messages, telemetry, etc, but Drone.isConnected(), returns false without that extra HEARTBEAT_FIRST message processing in there.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. So much indirection with the use of extends can make the code hard to follow! It's would almost be better if the GenericMavLinkDrone object received all messages first and then call a potentially derived processOnMavLinkMessageReceived implemented in specializations to help with the synchronization issues we are seeing. (i.e. the checkIfFlying call is obviously not happening on first connection before anything else)

@billbonney billbonney added the bug label Feb 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants