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

Mavgen JS: Heartbeat message mavlink_version should be automaticlly completed #926

Merged

Conversation

shancock884
Copy link
Contributor

@shancock884 shancock884 commented Mar 3, 2024

This PR updates the Javascript generator to automate the filling of the HEARTBEAT.mavlink_version parameter.
As per the C Mavgen, it uses 'omit_arg' parameter of the field to determine that the parameter should be omitted from the input parameter list, and 'const_value' parameter to determine the value to set.

When building a HEARTBEAT message via the constructor the parameter should no longer be specified, and the test cases have been updated to do this.
When the message is being read from a data stream, the value is correctly populated to whatever its actual value is, as this uses direct field assignment and not the constructor to build the message (I have tested this).

The change in output ends up looking like this:
from:

/*              ...
                system_status             : System status flag. (uint8_t)
                mavlink_version           : MAVLink version, not writable by user, gets added by protocol because of magic data type: uint8_t_mavlink_version (uint8_t)
*/
mavlink20.messages.heartbeat = function( ...moreargs ) {
[ this.type , this.autopilot , this.base_mode , this.custom_mode , this.system_status , this.mavlink_version ] = moreargs;

to:

/*              ...
                system_status             : System status flag. (uint8_t)
*/
mavlink20.messages.heartbeat = function( ...moreargs ) {
[ this.type , this.autopilot , this.base_mode , this.custom_mode , this.system_status ] = moreargs;
this.mavlink_version = 3;

Testing:
I have tested this on my React Native application, including checking that a received message correctly populates the field - even if its value differs from the default.
This is also tested in CI on a variety of NodeJS versions.

Remove from constructor parameter list, and assign directly.
Remains settable by the unpacker for received messages.
Heartbeat test cases updated to reflect new interface.
@shancock884 shancock884 marked this pull request as ready for review March 3, 2024 14:22
@peterbarker
Copy link
Contributor

Looks fine to me, but @davidbuzz will know better than I.

Copy link
Contributor

@davidbuzz davidbuzz left a comment

Choose a reason for hiding this comment

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

looks good to me, thankyou.! - @peterbarker happy for you to merge

@davidbuzz
Copy link
Contributor

davidbuzz commented Mar 14, 2024

@shancock884 -if your're using the Javascript generator in a project, I'd love to hear about it.. eg, I've used it in a few projects... for example, I implemented this tool... which is the only one i know of that supports packet-signing with these bindings... https://github.com/davidbuzz/MAVAgent

@peterbarker peterbarker merged commit e0caa6f into ArduPilot:master Mar 15, 2024
12 checks passed
@peterbarker
Copy link
Contributor

Merged, thanks!

@shancock884 shancock884 deleted the mavgen-js-heartbeat-mav-ver branch March 15, 2024 07:56
@shancock884
Copy link
Contributor Author

@shancock884 -if your're using the Javascript generator in a project, I'd love to hear about it..

Hi @davidbuzz. The short version is that I have been working on a mobile phone app for planning waypoint routes - initially just for the drone that our startup (Linking Drones) is working on, using bluetooth. Then to expand its scope of usefulness I was looking into using the phone's USB port as a wired connection to any Mavlink drone. As the app is using React Native I needed some JS!
If you have any follow-ups, feel free to message or email!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants