MavLinkDrone: Fixes #443 and makes sysid/compid use the expected shor…

[billbonney]

…t type

sysid/compid need to use short with the current library and not byte as it stores the values as an unsigned 0-255 value in the short variable. Missing the conversion will cause a crash.

This PR can serve as a discussion as to the correct fix. But I think it should be along the lines that mysid/compid needs to be represented as short (like in the library) or the java mavlink lib needs to represent unsigned bytes as signed bytes and conversion only happens when the byte is displayed to the user.

Exception is caused here

https://github.com/dronekit/dronekit-android/blob/develop/dependencyLibs/Mavlink/src/com/MAVLink/Messages/MAVLinkPayload.java#L140

When the target system id (or component id) is -127 to -1 i.e. 128-255)

dronekit-android/dependencyLibs/Mavlink/src/com/MAVLink/common/msg_request_data_stream.java

Line 62 in 4f965d9

packet.payload.putUnsignedByte(target_system);

[m4gr3d]

@billbonney The HeartBeat#sysid and HeartBeat#compid field are coming in as int and being downcast to byte. Looks to me we should change them to short as well:

dronekit-android/ClientLib/src/main/java/org/droidplanner/services/android/impl/core/drone/variables/HeartBeat.java

Lines 67 to 68 in 0f1e038

	sysid = (byte) msg.sysid;
	compid = (byte) msg.compid;

[m4gr3d]

Fixes issue #443

[billbonney]

Interestingly it almost like data integrity needs to be abstract class MAVLinkMessage to be short and in the valid 0-255 range, since at the moment these casts hide any problems. I'll think about it some more on how best to fix... (I'm almost thinking that just being real byte and only deciphering when its a to be displayed would have been more sensible) thinking some 🤔

[m4gr3d]

@billbonney I thought about that as well, but then you'd have to decipher it everywhere you use the value, vs using a larger type everywhere, and only worry about encoding it once before you send it over the wire.

[billbonney]

I've added a slightly different fix, which validate the mysid/comid range, but really it's in the wrong place. see 7e7d1b0

The real problem is that this class https://github.com/dronekit/dronekit-android/blob/develop/dependencyLibs/Mavlink/src/com/MAVLink/Messages/MAVLinkMessage.java#L22
Should allow public access to the mysid/compid/msgid and really should have validators to make sure they are always set to be 0-255 and this problem would have been caught earlier.

As i figure the code out again, i'll see if I can fix it at source and then it should alert us to other problematic areas (if any) with casts that just don't do the right thing

[billbonney]

I added this check, so that an invalid conversion would break, but since this comes over the wire it would be safe to cast from int to short. I think where the java library goes wrong is that it uses int in the first place. if it used short these issue would have shown up, as there would be no need to cast and therefore mask the problem.

[billbonney]

I've made the changes you requested. Since it now force the sys_id and comp_id to be treated as short (and it should be always to avoid problems.)

I'm not sure about adding the validate method to DroneParameter. comments welcome. thx :)

[m4gr3d]

@billbonney I agree the fix should be made at the source, which in this case would be the mavlink Parser class.
That class is autogenerated from the java mavlink library generator, so the fix needs to be applied to the original template, and the mavlink library needs to be regenerated to propagate the fix:
https://github.com/ne0fhyk/mavlink/blob/d543345a2712bb3c3f466bfddd9eef5ecf0a3d19/pymavlink/generator/java/lib/Parser.java#L64-L75

[billbonney]

@ne0fhyk The problem with the byte ranges is that they are fine for the parsing the incoming protocol ( a byte is a byte ;) ).

After some more thought the problem was only with the container classes with sys_id/comp_id not using the 'short is a byte' convention and that was root of the error. I'll update this PR later, and i think we will be good for now with those changes

Later: I should explain what I mean by source. I mean that it should eb consistent throughs he code that comp/sys_id is short. In MAVLinkPacket it is an int

[m4gr3d]

@billbonney sounds good. I'll merge the current pr in order to fix issue #443, and create another issue for the mavlink library for the full update.

[m4gr3d]

lgtm

[billbonney]

@ne0fhyk Great

I was thinking that if we do update the MAVLinkPacket object, making it so that sys_id/comp_id are set using a setter, would allow for the check for validity to be performed easily.