Add option to send MAVLink heartbeat messages #170
Conversation
|
Nice! I'll have time later in the week to take a look. |
There was a problem hiding this comment.
Thanks for this - I've done a review on the specific (small) issues).
More generally:
- Can you switch to using spaces-for-tabs, to maintain consistency with the rest of the codebase
- There's some failing tests that need to be looked at
- For any existing functions in
mavManagethat usesendData, default them to using theminimal.MavComponent.ONBOARD_COMPUTERcomponentID for consistency.
mavlink/mavManager.js
Outdated
| @@ -240,6 +247,28 @@ class mavManager { | |||
| }) | |||
| } | |||
|
|
|||
| sendHeartbeat ( type = mavheader.MAV_TYPE_ONBOARD_CONTROLLER, | |||
There was a problem hiding this comment.
type = minimal.MavType.ONBOARD_CONTROLLER
mavlink/mavManager.js
Outdated
| @@ -240,6 +247,28 @@ class mavManager { | |||
| }) | |||
| } | |||
|
|
|||
| sendHeartbeat ( type = mavheader.MAV_TYPE_ONBOARD_CONTROLLER, | |||
| autopilot = mavheader.MAV_AUTOPILOT_INVALID, | |||
There was a problem hiding this comment.
autopilot = minimal.MavAutopilot.INVALID
mavlink/mavManager.js
Outdated
| heartbeatMessage.systemStatus = systemStatus | ||
| heartbeatMessage.mavlinkVersion = this.version | ||
|
|
||
| this.sendData(heartbeatMessage, system, mavheader.MAV_COMP_ID_ONBOARD_COMPUTER) |
There was a problem hiding this comment.
minimal.MavComponent.ONBOARD_COMPUTER
mavlink/mavManager.js
Outdated
| baseMode = 0, | ||
| customMode = 0, | ||
| systemStatus = 0, | ||
| system = this.targetSystem, |
There was a problem hiding this comment.
this.targetSystem and this.targetComponent are the flight controller (the target) not Rpanion. Since Rpanion is (assumed to be) on the vehicle, it makes sense to use this.targetSystem. But we'll need our own component ID, not the flight controller's component ID
I think you meant for the component argument to be passed down to line 269? Then use the default component here as minimal.MavComponent.ONBOARD_COMPUTER
There was a problem hiding this comment.
Yes, I had changed line 269 to have the component argument passed down but somehow it didn't make it into this commit. Fixed.
mavlink/mavManager.js
Outdated
| @@ -213,17 +215,22 @@ class mavManager { | |||
| this.udpStream.bind(this.inudpPort, this.inudpIP) | |||
| } | |||
|
|
|||
| sendData (msg) { | |||
| sendData (msg, vehicle = 255, component = 1) { | |||
There was a problem hiding this comment.
Since we can safely assume the Rpanion will always be on the vehicle, just use this.targetSystem instead of an argument in the function.
|
Thanks for the review! I believe the latest changes to the branch will resolve the issues that you pointed out, and I've changed my editor setup to hopefully stay consistent with tab formatting. |
|
Thanks for the changes. There are a few tabs-for-spaces in the PR which you missed. I missed in my review that I'd like the "Enable Heartbeats" checkbox to be moved down to the "Other Options" section. Could you also flatten the PR, so it doesn't have any merges? |
ddd999
force-pushed
the
heartbeat
branch
4 times, most recently
from
5f64149 to
4ecee29
Compare
I think I have taken care of all that now. I'm definitely still learning here so thanks for your patience. |
|
There appear to be some references to a "cameracontroller.js" in |
|
Sorry about that. I thought I had fixed that particular mess-up, but I guess missed a couple of spots. Should be fixed now. |
|
Great. That all looks good! Merging... |
This allows Rpanion to send out heartbeat messages advertising itself on the MAVLink network as a companion computer which is a component of the vehicle.
The changes to sendData() and the addition of sendHeartbeat() are a first step toward being able to respond to MAVLink commands, and advertising other devices that can accept commands (i.e., cameras).