Add MAVLink receiver support, RADIO_RC_CHANNELS #24577

olliw42 · 2023-08-10T13:10:19Z

This is the first piece of adding what should be called a "MAVLink receiver".

This PR requires that the storm32.xml mavlink dialect file in https://github.com/ArduPilot/mavlink/tree/master/message_definitions/v1.0 is updated to that in the main mavlink repository.

The idea is simple, namely a receiver which does send its RC channel data (and other data such as link statistics) not via common protocols like SBus or CRSF but via a MAVLink message. Since it's such an obvious concept and so much desired, at least in the hobby world, I don't really know what else to say to make the case.

A deeper analysis of the situation and description of the underlying concept can be found here Proposal for mavlink radio links & receivers: Concept and set of new messages mavlink/mavlink#1921.
I note that the very same is been done for DroneCAN, sensors: add RCInput message dronecan/DSDL#26, and the same arguments should apply here. It would be ironical that there would be a case for a CAN receiver but not a MAVLink receiver.
A follow up PR would implement support for a RADIO_LINK_STATS message as second piece. While not formally approved by MAVLink it was discussed and seem to at least not have thrown out of the window: https://github.com/mavlink/mavlink/wiki/20230719-Dev-Meeting

This message is implemented since in a while in my BetaPilot/Copter fork as well as in the mLRS project which provides a practical example of a MAVLink receiver, and also establishes a proof of concept, as well as demonstrates that the code PR'ed here works (in the setups used by those who tested/use it).

The RADIO_RC_CHANNELS mavlink message is currently located in the storm32.xml with ID 60045, but I'm happy to remove it from there and move it to any other location with any other ID you guys would suggest.

magicrub · 2023-08-11T19:22:46Z

The PR for the proposed message is mavlink/mavlink#1919 which would need to be added to ArduPilot's mavlink fork too.

olliw42 · 2023-08-14T09:01:03Z

The PR for the proposed message is mavlink/mavlink#1919 which would need to be added to ArduPilot's mavlink fork too.

yes, something needs to be done in the ArduPilot's mavlink fork. It is frankly however not so clear (to me) what the best action would be; I see a couple of approaches. I admit that I'm also not totally sure (lack of knowledge) how the technical aspect of handling changes in submodules would work here.

I think it would be very useful if you guys could come up with an opinion - or even decission - if

you want to support a MAVLink receiver along the lines suggested here
you are ok with the design of RADIO_RC_CHANNELS
you are generally ok with the implementation approach taken in this PR

Maybe you find a chance to briefly discuss this in a dev call.

THX!

olliw42 · 2023-11-27T09:23:02Z

I'd like to ask again if you maybe could find time to briefly discuss this topic in general and come up with an opinion or even decission on it.

I continue to argue, and see more and more signs pointing towards it, that this is just what "many" users would want to have, one connection for both mavlink and rc handling.
("many" users may not include your partners and other commercially oriented parties, which certainly are an most important driving force in ArduPilot, but the more casual and hobby users, which is a still existing group of users).

What would be helpful to know is your general opinion. Like
[] we like the concept and idea of a "mavlink receiver" and would be interested in it, but the PR sure needs work
[] we like the concept and idea of a "mavlink receiver" but don't consider it of interest for Ardupilot
[] we think the concept and idea of a "mavlink receiver" is flawed and is not suitable for Ardupilot

This PR is broken and needs an addition downstream in mavlink/pymavlink, sure, but easy enough. The question I wonder about is if it is worth to spend the effort. I'd be very happy to spend the effort, iff there is slight indication it may work out.

Thx.

jlpoltrack · 2023-11-27T12:40:05Z

IMO, think that this makes a lot of sense and should be explored. As wireless links evolve it seems ideal to have a single link that can handle both MAVLink + RC.

Ryanf55 · 2023-11-27T23:18:51Z

modules/Micro-XRCE-DDS-Client

Please remove the unrelated and likely unintentional changes to submodules like the XRCE Client lib.

Try git checkout origin/master -- modules/Micro-XRCE-DDS-Client assuming your remote is origin

MANY thx. Found a different workaround in the mean time. Thx again :)

Ryanf55

Unrelated changes need removed prior to merge.

tridge

needs an autotest, and needs cleanup of the submodule changes (remove ChibiOS and XRCE submodule changes)

on your question, the concept is good I think, just needs a bit of re-work

tridge · 2023-11-27T23:18:48Z

libraries/GCS_MAVLink/GCS.h

@@ -746,6 +747,9 @@ class GCS_MAVLINK
    mavlink_message_t _channel_buffer;
    mavlink_status_t _channel_status;

+    // for mavlink radio
+    mavlink_radio_t _mavlink_radio_packet;


better to make this a pointer and allocate on first receiver so we don't pay for it on every connection

this is actually not needed for this PR but a leftover. Will be needed for a followup PR doing link statistics. Field removed.

tridge · 2023-11-27T23:19:39Z

libraries/GCS_MAVLink/GCS_Common.cpp

@@ -6407,6 +6410,12 @@ bool GCS_MAVLINK::accept_packet(const mavlink_status_t &status,
        return true;
    }

+    // handle messages from a mavlink receiver
+    if ((msg.compid == MAV_COMP_ID_TELEMETRY_RADIO) &&
+        (msg.msgid == MAVLINK_MSG_ID_RADIO_RC_CHANNELS)) {


this bypasses signing checks, for a packet that can control the vehicle, this is very dangerous, we must not bypass those checks

I 100% agree that signing checks should not be bypassed, but I can't see anything in the function accept_packet() which would do something related to signing.
Could you maybe drop a hint of what I'm missing?

I'll need to look when not on my phone, I thought signing used this for a bypass check

libraries/GCS_MAVLink/GCS_Common.cpp

tridge · 2023-11-27T23:22:03Z

libraries/GCS_MAVLink/MAVLink_routing.cpp

@@ -140,6 +140,13 @@ bool MAVLink_routing::check_and_forward(GCS_MAVLINK &in_link, const mavlink_mess
    int16_t target_component = -1;
    get_targets(msg, target_system, target_component);

+    // handle messages from a mavlink receiver as if it had target_system = our system, target_component = 0
+    if ((msg.compid == MAV_COMP_ID_TELEMETRY_RADIO) &&


does the msg not have target_system?

no. Pl see the post on this below. #24577 (comment).

This precludes having redundant receivers on-board.

You can target the message easily enough - we use the same technique on things like ADSB receivers. If no explicit target has been specified on the device then it waits for a heartbeat to arrive and (after filtering for appropriate vehicle type and MAV_TYPE). It then locks onto the flight controller, targetting it and using a source system the same as that flight controller. component ID can default to TELEMETRY_RADIO but should also be configurable (and that ID should have no relevance to the way the packet is processed on the flight controller, to allow for redundancy)

peterbarker

rename files for case sensitivity (i.e. MAVLink not Mavlink)

I think I like this concept.

libraries/AP_RCProtocol/AP_RCProtocol.h

libraries/AP_RCProtocol/AP_RCProtocol.cpp

libraries/GCS_MAVLink/GCS_Common.cpp

peterbarker · 2023-11-27T23:22:31Z

libraries/GCS_MAVLink/MAVLink_routing.cpp

+    if ((msg.compid == MAV_COMP_ID_TELEMETRY_RADIO) &&
+        (msg.msgid == MAVLINK_MSG_ID_RADIO_RC_CHANNELS)) {
+        target_system = mavlink_system.sysid;
+        target_component = 0;
+    }


This doesn't look great

Add fields to the packet for target system/component.

concerning targets, pl see post below. #24577 (comment).

tridge · 2023-11-27T23:24:29Z

@olliw42 please ping on code-review discord channel when ready for re-review

wvarty · 2023-11-28T07:35:10Z

This work looks like it may also be helpful for the in-flight dev work to add mavlink as a serial protocol to ELRS.
I think the most valuable contribution will be the link stats though (suggested as a subsequent PR in the description), which is a slightly more complicated topic, as things like RSSIdBm and SNR are missing as OSD elements in mainline ardu.

olliw42 · 2023-11-28T08:17:20Z

first off, many thx for the positive general response, this is very helpfull and encouraging.

@tridge @peterbarker

on the topic of target fields for this message.

It does not have any. The reasons are outlined in the first post of mavlink/mavlink#1921, in the two subchapters "Routing", and in last post in its last subchapter.
In a nutshell: These target fields would have to be populated with values, which the receiver thus would need to know somehow. I can't see a reasonable way how it could auto-determine them, and explicit configuration via a parameter is anything than user friendly, IMHO.
Not using targets comes with cons, but I judged the overall balance of cons vs pros to be better than for other options which came to my mind.

This is a very relevant "detail" and should be done right, and thus discussed well.

khancyr · 2023-11-28T08:49:03Z

You can put the target field.
If they aren't used, they will be at 0 that will be broadcast in mavlink routing, so that is the same as not having the field.
But as the fields are presents, those that have ability to manage parameters would be able to benefit of them.
That shouldn't collide with the way you propose to use the message I think

olliw42 · 2023-11-28T08:52:35Z

This PR needs an associated PR to https://github.com/ArduPilot/mavlink in order to bring in the RADIO_RC_CHANNELS message.

Which is a serious problem for me. It seems that github does not allow to have forks of both the main repo and a fork of that main repo. Unfortunately I do have a fork of https://github.com/mavlink/mavlink, of which https://github.com/ArduPilot/mavlink is a fork of. It seems I'm doomed.

What can I do?

In the meanwhile, I have rebased and updated the respective PR to the main repo: mavlink/mavlink#1919. I also add a post there listening the discussion points, like the targets topic.

olliw42 · 2023-11-28T09:41:35Z

You can put the target field. If they aren't used, they will be at 0 that will be broadcast in mavlink routing, so that is the same as not having the field. But as the fields are presents, those that have ability to manage parameters would be able to benefit of them. That shouldn't collide with the way you propose to use the message I think

I sure have thought about this but found it not to achieve the desired goals, that is, comes with costs which outweight the benefits. IMHO.

In my mind the message should "exist" only on the system, but not outside of it. However, if it is generally considered acceptable that these messages are normally broadcasted, even beyond the system, then there is no issue and I'm trying to solve a non-issue.

I could reproduce here now the arguments and discussion in the linked posts,but I'm not sure how helpfull it would be to reproduce all arguments, in essentially just different words. Maybe I can kindly ask you to read the two respective paragraphs in the first post of mavlink/mavlink#1921. I have just reread them and find then sufficiently ok.

The reasons IMHO are largely the same reasons for why you find special code for MAVLINK_MSG_ID_RADIO_STATUS, MAVLINK_MSG_ID_RADIO, like here https://github.com/ArduPilot/ardupilot/blob/master/libraries/GCS_MAVLink/MAVLink_routing.cpp#L117-L121 or here https://github.com/ArduPilot/ardupilot/blob/master/libraries/GCS_MAVLink/GCS_Common.cpp#L6405-L6408.

The extend, let me mention few things which are different to a normal MAVLink component (autopilot, gimbal, companion,...)

The receiver has a system_id different from both the GCS (or GCSes) and the autopilot (or autopilots). Generically its system_id would be 51 like for the SiK radios.
The receiver is normally not emitting a HEARTBEAT message (it could and would have to if it wants to support e.g. the parameter service, but it should not just for the sake of being a receiver).

Each of these point can be debated, with pros and cons.

Note also that the RADIO_RC_CHANNELS message (and others of this set not part of this PR) must not be emitted by anything else than a mavlink receiver (without this constraint it is indeed easily possible to create all sorts of situations producing conflicts)

khancyr · 2023-11-28T13:25:07Z

I have read your other post before answering.
What you are proposing, and it is fine to me, is a special handling for this message and prevent it to be fowarded (we will need to check this carefully). Fine on this point.
How are you doing this ? We got a new message that don't have target system. So in mavlink routing perspective that is a implicitly a broadcast message by default. Do we agree on that ?

Normally, your intend for this message is that it is send directly from the receiver to whatever will handle its msg content and should prevent the forwarding by the special handling ? I got it right ?

What I say is : instead of doing implicity mavlink broadcast, add the target field. It will do explicit mavlink broadcast. Then we can still apply the special handling. It shouldn't change anything on your case and PR.
But for those that want to do targeting and forwarding, they can do it now. So we can have unparametrized receiver and parametrized one's using the same message. Is my reasoning clear enough and right ?

olliw42 · 2023-11-28T16:58:03Z

many thx for your further comments. Your reasoning is clear, but I do have a bit of a different perspective on it.

We got a new message that don't have target system. So in mavlink routing perspective that is a implicitly a broadcast message by default. Do we agree on that ?

absolutely

Normally, your intend for this message is that it is send directly from the receiver to whatever will handle its msg content ...? I got it right ?

here I divert. I read "send directly" to imply a fully targeted message, going just from the receiver to exactly one other endpoint (target_sysid != 0 and target_compid != 0 in mavlink routing speach). I instead think that normally it should be send to all components of the system, autopilot, gimbal, companion, camera, .... whatever. That is target_sysid != 0 and target_compid == 0, in mavlink routing speach.

Then we can still apply the special handling.

I agree with you in adding the target fields in so far as it is better to add them now for just in case. One drawback of the design of RADIO_RC_CHANNELS is that it can't be extended later (at least not without dismaying cost). So, any mistake now will bite later. In that sense it's better to just add them.

If you guys would not mind having the speical handling, problem seems to be solved.

However: Let's think for a moment about how it would read in the standard's docs. I mean, eventually the message should become standard and move into common.xml, right. We would essentially suggest to write: "The message has target fields but these should not be normally used and in fact each implementation should add special code to handle the message properly in the normal cases."? I would find that pretty odd in terms standardization, and I have doubts it would run well.

Also, if that special code is there, you can't get around it by pure mavlink means (i.e. not changing the code), so the target_sysid field will be irrelevant, and all you could do is to play with target_compid.

I want to mention again that there never ever should be a situation there one would send that message to anything else than components of the system it is connected to. (in particular it must not send e.g. from the ground to the vehicle, or from the vehicle to the ground). That is target_sysid must always be that of the system. So in the docs we actually would write "The message has target fields but these should not be normally used and in fact each implementation should add special code to handle the message properly in the normal cases. In addition target_sysid must be that of the system the receiver is connected to".

In my mind the two things "special code to handle routing" and "target fields for normal mavlink routing" are kind of mutually exclusive options.

I will not mind all that, if that is what the preference is. I'm just not convinced. :)

tridge · 2023-11-28T23:24:03Z

@olliw42 ok, I understand and agree on why it doesn't have target fields

tridge

I'm stuck on my phone for this review, so hard to look in detail sorry

tridge · 2023-11-28T23:28:35Z

libraries/AP_RCProtocol/AP_RCProtocol.cpp

+    }
+
+    // update the field, so that the backend can see it
+    memcpy(&(mavlink_radio.rc_channels), packet, sizeof(mavlink_radio_rc_channels_t));


Can this be a structure assignment instead of memcpy?

added virtual handlers to backend, which makes this field and line go altogether. Done.

tridge · 2023-11-28T23:30:50Z

libraries/AP_RCProtocol/AP_RCProtocol.h

+    // handle mavlink radio
+    void handle_radio_rc_channels(const mavlink_radio_rc_channels_t* packet);
+    struct {
+        mavlink_radio_rc_channels_t rc_channels;


This seems to be chewing a chunk of ram per instance? We have a lot of instances

is located in the frontend, so should be "just" one of them, as I understand it.
Very usefull comment. Added virtual handlers to backend, which makes this field go altogether. Nice. Done.

tridge · 2023-11-28T23:32:14Z

libraries/AP_RCProtocol/AP_RCProtocol_MavlinkRadio.cpp

+
+        uint16_t rc_chan[MAX_RCIN_CHANNELS];
+        for (uint8_t i = 0; i < count; i++) {
+            rc_chan[i] = ((int32_t)frontend.mavlink_radio.rc_channels.channels[i] * 5) / 32 + 1500;


A comment explaining this maths would be nice

nice. done.

tridge · 2023-11-28T23:33:29Z

libraries/AP_RCProtocol/AP_RCProtocol_config.h

@@ -61,3 +61,7 @@
 #ifndef AP_RCPROTOCOL_FASTSBUS_ENABLED
 #define AP_RCPROTOCOL_FASTSBUS_ENABLED AP_RCPROTOCOL_BACKEND_DEFAULT_ENABLED && AP_RCPROTOCOL_SBUS_ENABLED
 #endif
+
+#ifndef AP_RCPROTOCOL_MAVLINK_RADIO_ENABLED
+#define AP_RCPROTOCOL_MAVLINK_RADIO_ENABLED AP_RCPROTOCOL_BACKEND_DEFAULT_ENABLED


Maybe default enable only on 2M flash boards? I haven't looked at flash cost of the feature

tridge · 2023-11-28T23:34:38Z

libraries/GCS_MAVLink/GCS_Common.cpp

@@ -6407,6 +6410,12 @@ bool GCS_MAVLINK::accept_packet(const mavlink_status_t &status,
        return true;
    }

+    // handle messages from a mavlink receiver
+    if ((msg.compid == MAV_COMP_ID_TELEMETRY_RADIO) &&
+        (msg.msgid == MAVLINK_MSG_ID_RADIO_RC_CHANNELS)) {


I'll need to look when not on my phone, I thought signing used this for a bypass check

olliw42 · 2023-11-29T09:00:02Z

~~I haven't flight tested the last round of changes yet. Will do ASAP.~~ all good, passed.

olliw42 · 2023-11-30T09:40:47Z

@tridge
if nothing new comes up it seems two points are left:

settle signing concerns
* how to bring in RADIO_RC_CHANNELS message (or how to deal with both a fork and a fork of a fork in github)

olliw42 · 2023-12-03T20:30:52Z

it would be much appreciated if you maybe could find time again to briefly discuss this PR in the dev call. It seems to me it has made progress, and it would be sad if it couldn't be finished. Many thx in advance.

olliw42 · 2023-12-06T15:41:18Z

@jlpoltrack thankfully volunteered to submit the PR for merging the RDAIO_RC_CHANNELS message into your mavlink repo: ArduPilot/mavlink#343.

so, the point which is left to settle are the signing concerns.

olliw42 · 2023-12-09T10:53:12Z

Addendum:
I want to mention that the lines which raised security concerns in signing situations, https://github.com/ArduPilot/ardupilot/pull/24577/files#diff-a8cfae886a15156d6cab6c16716a6081f6d0f3fe8a5d730a3f23ab6f740fcfecR6415-R6422, are not really relevant nor crucial for the message and concept to work.
That is, I would not hesitate for a second to remove them if there are any concerns with them which can't be settled now.
I added them as they made sense to me, but think that most users who would be interested in using a mavlink receiver as of today won't have SYSID_ENFORCE set anyway, so no harm done with removing the lines.
If that helps to draw a decission sooner than later :)

Addendum II:
It might have be the function accept_unsigned_callback() (https://github.com/ArduPilot/ardupilot/blob/master/libraries/GCS_MAVLink/GCS_Signing.cpp#L114-L127) which might have been in mind when suspecting the singing issue in the function accept_packet(). It also has special treatment for RADIO_STATUS.

olliw42 · 2023-12-11T06:32:43Z

@peterbarker @khancyr @tridge devcall maybe?
:)

peterbarker

This is looking good, I just have concerns about the lack of targetting.

My major concern on the lack of targetting of the mavlink message is that (a) it's relatively easily solveable in a user-friendly manner, and that if you've got swarms of vehicles seeing each other's traffic you really don't want vehicles seeing other vehicle's messages! If you have your RC receiver feeding into e.g. cmavnode - then you're likely to get that sort of problem!

peterbarker · 2023-12-13T08:15:58Z

libraries/GCS_MAVLink/MAVLink_routing.cpp

@@ -140,6 +140,13 @@ bool MAVLink_routing::check_and_forward(GCS_MAVLINK &in_link, const mavlink_mess
    int16_t target_component = -1;
    get_targets(msg, target_system, target_component);

+    // handle messages from a mavlink receiver as if it had target_system = our system, target_component = 0
+    if ((msg.compid == MAV_COMP_ID_TELEMETRY_RADIO) &&


This precludes having redundant receivers on-board.

You can target the message easily enough - we use the same technique on things like ADSB receivers. If no explicit target has been specified on the device then it waits for a heartbeat to arrive and (after filtering for appropriate vehicle type and MAV_TYPE). It then locks onto the flight controller, targetting it and using a source system the same as that flight controller. component ID can default to TELEMETRY_RADIO but should also be configurable (and that ID should have no relevance to the way the packet is processed on the flight controller, to allow for redundancy)

libraries/AP_RCProtocol/AP_RCProtocol.cpp

olliw42 · 2023-12-13T09:54:11Z

This precludes having redundant receivers on-board.

not necessarily, the different receivers could be given different system id's. I agree however that this is not what one may like, but fortunately MAVLink provides a canonical pattern for how to handle that.

One should define multiple MAV_COMP_ID_TELEMETRY_RADIO, i.e. also in additon MAV_COMP_ID_TELEMETRY_RADIO2, MAV_COMP_ID_TELEMETRY_RADIO3, ...

These are not currently existing in MAVLink. But could be added any time, on desire.

My major concern on the lack of targetting of the mavlink message is that (a) it's relatively easily solveable in a user-friendly manner

You can target the message easily enough - we use the same technique on things like ADSB receivers. If no explicit target has been specified on the device then it waits for a heartbeat to arrive and (after filtering for appropriate vehicle type and MAV_TYPE). It then locks onto the flight controller, targetting it and using a source system the same as that flight controller. component ID can default to TELEMETRY_RADIO but should also be configurable (and that ID should have no relevance to the way the packet is processed on the flight controller, to allow for redundancy)

we are approaching a limit cycle. I had explained why it's not a good option to having to wait for a heartbeat.

I very well know this mechanism and use it e.g. also in the STorM32 controller and it's working great there too. But not so well here.

Let me try again. In this approach the rc channels data are send out only after a flight controller has been detected. This is - IMHO - absolutely not hwat one wants, as a user. E.g.,

one could not test that receiver without an attached working fc
other components such as gimbals or cameras which may use the message and its data could not start working with that data before the fc has booted up and revealed itself
one could not test these other component's rc data handling without an attached working fc
one could not use that receiver without a fc in the system or conncetion to that fc. E.g. one could not attach such a receiver to a STorM32 gimbal for dual operation (without nasty code tricks).
... I propbably missed more "little" annoyences comming from this

I just see a bag full of inconveniences. Let's put it that way: If that would be what the final result would be I would actually be totally not interested in using such a receiver...

and that if you've got swarms of vehicles seeing each other's traffic you really don't want vehicles seeing other vehicle's messages! If you have your RC receiver feeding into e.g. cmavnode - then you're likely to get that sort of problem!

that's why there are these routing rules which say that the message should be routed as if it had target_system = system id and target_component = 0 so as to not ever go beyond it's attached system. :)

…NELS message

peterbarker · 2023-12-26T22:28:03Z

This precludes having redundant receivers on-board.

not necessarily, the different receivers could be given different system id's. I agree however that this is not what one may like, but fortunately MAVLink provides a canonical pattern for how to handle that.

One should define multiple MAV_COMP_ID_TELEMETRY_RADIO, i.e. also in additon MAV_COMP_ID_TELEMETRY_RADIO2, MAV_COMP_ID_TELEMETRY_RADIO3, ...

I absolutely do not subscribe to the MAV_COMP_ID_* concept of allocating functions to IDs. HEARTBEAT tells you what sort of thing a mavlink node is, and with only a 255-entry namespace we are so going to run out why the time you add enough IDs for as many things as you could potentially have on random vehicles.

My major concern on the lack of targetting of the mavlink message is that (a) it's relatively easily solveable in a user-friendly manner

You can target the message easily enough - we use the same technique on things like ADSB receivers. If no explicit target has been specified on the device then it waits for a heartbeat to arrive and (after filtering for appropriate vehicle type and MAV_TYPE). It then locks onto the flight controller, targetting it and using a source system the same as that flight controller. component ID can default to TELEMETRY_RADIO but should also be configurable (and that ID should have no relevance to the way the packet is processed on the flight controller, to allow for redundancy)

we are approaching a limit cycle. I had explained why it's not a good option to having to wait for a heartbeat.

I'm sorry, I must have missed that argument somewhere. Can you point that out somewhere, please?

I very well know this mechanism and use it e.g. also in the STorM32 controller and it's working great there too. But not so well here.

Let me try again. In this approach the rc channels data are send out only after a flight controller has been detected. This is - IMHO - absolutely not hwat one wants, as a user. E.g.,
* one could not test that receiver without an attached working fc

Or manually configuring a target system/component on the unit.

* other components such as gimbals or cameras which may use the message and its data could not start working with that data before the fc has booted up and revealed itself

And if users want to use a separate RC receiver for the camera vs the flight controller? In the case you here you'd need to configure the source system manually on the device providing hte mavlink message. Also a good use of target system/component - but does require configuration

* one could not test these other component's rc data handling without an attached working fc

Not without configuring the source system manually. Configurability of the source component is important to allow for redundancy.

* one could not use that receiver without a fc in the system or conncetion to that fc. E.g. one could not attach such a receiver to a STorM32 gimbal for dual operation (without nasty code tricks).

.. no, just configuration. 99% of cases out there you plug-and-play. 1% of the cases require (and probably desire!) configuration.

that's why there are these routing rules which say that the message should be routed as if it had target_system = system id and target_component = 0 so as to not ever go beyond it's attached system. :)

We shouldn't need to add routing rules for new messages, and "legacy" software NOT having this filtering/routing (root cause of tacking routing onto MAVLink based on magic field names/explicit listing) mean that these messages would leak and create havoc.

olliw42 · 2023-12-28T09:19:46Z

many thx for taking your time and looking at it again, but I think it's the type of discussion I don't want to be in.

Concerning heartbeat and type and id, I normally couldn't agree more, and I believe that some years back it was me who worked hard to convince the mavlink folks that this is how it has to be, and get respective language into the docs. Yet for the mavlink receiver I came (and still come) to a different conclusion, IMHO it just adds loads of complexity for no purpose except of clinging to a principle, using a sledgehammer to crack a nut.

It's a weired discussion. There is absolutely nothing in ArduPilot which would adhere to that mavlink principle you've mentioned, to the contrary. I just looked up again how some mavlink components are handled, to reasure that's accurate. For mavlink components like GPS, optical flow, proximity, rangefinder, OSD, the handling is as crude as it can be, and far off any principle like the one you suggested. Gimbal and camera are somewhat better, since here there is indeed a find phase, yet it's strictly required to use predefined gimbal and camera ids, in obvious contrast to your position. The greemsy driver (and possibly others) breaks mavlink principles also in other fundamental ways.

So, frankly, it feels like been drawn into a discussion which ArduPilot hasn't settled for itself, and most of the time is happy to violate, and all what is clear to me is that I don't want to be in that discussion.

Concerning ease of use, user friendliness, we are not going to share common ground. All this configuration talk may indeed look easy and natural to you, but it's a reason for the sentiment that ArduPilot is - comparatively - complicated to set up and use. ArduPilot's mindset doesn't appear to match up with most user's idea of ease of use, and to be frank, I here will trust my own judgment much more than that of any ArduPilot member.

So, where to go from here.

Your comments imply that the mavlink receiver has to also emit a heartbeat, and that the discovery of that component should be based on it and moreover be untight from any predefined ids. If that is the position, I'd like to say that it's not me who is going to pursue this or even try to implement this. In that case, it probably would be appropiate to just close that PR.

If however all we are talking about is the routing and target id thing, i.e. no heartbeat and type and etc. pp, then we may have a deal. It's easy enough to modify the above code and the xml definition, and I will be fine with doing this (see #25838 for how it may look like).

To sum up, I see two options:

close the PR and forget the thing
remove routing code and add target ids to xml definition

auturgy · 2023-12-28T21:49:31Z

@peterbarker I find myself on the fence with this: as we've discussed, I agree that ensuring that the implementation covers as many use cases as possible is important, but I think we also need to ensure that we don't let perfect be the enemy of good. Would getting this in as a compile/custom build server option provide sufficient protection for you? I think that if we do that, it provides both a safety net and a base to continue to evolve this.

olliw42 · 2024-01-13T07:26:28Z

@peterbarker @auturgy what's up?
I had changed the message to include targets

here the revised PR Add MAVLink receiver support, RADIO_RC_CHANNELS, no II #25838
here the PR to the AP mavlink repo development.xml: RADIO_RC_CHANNELS message mavlink#343

no good? dead horse?

peterbarker

Very partial review only; I was only looking for the handling of the failure flags

peterbarker · 2024-01-24T01:29:39Z

libraries/AP_RCProtocol/AP_RCProtocol_MavlinkRadio.cpp

+        rc_chan[i] = ((int32_t)packet->channels[i] * 5) / 32 + 1500;
+    }
+
+    bool failsafe = (packet->flags & RADIO_RC_CHANNELS_FLAGS_FAILSAFE);


This is not sufficient. If the values are frozen we don't want to use them either!

I'm not sure what you mean by "froozen"
the case when frames are not received is handled by the upper level code
if you mean handling missing frames bit: IMHO this is exactly as for sbus

magicrub added DevCallTopic DevCallEU labels Nov 27, 2023

olliw42 force-pushed the owpr-radiorcchannels branch from 65e6da7 to ba71fd6 Compare November 27, 2023 18:44

CraigElder removed the DevCallTopic label Nov 27, 2023

Ryanf55 reviewed Nov 27, 2023

View reviewed changes

Ryanf55 requested changes Nov 27, 2023

View reviewed changes

CraigElder removed the DevCallEU label Nov 27, 2023

tridge requested changes Nov 27, 2023

View reviewed changes

peterbarker requested changes Nov 27, 2023

View reviewed changes

olliw42 force-pushed the owpr-radiorcchannels branch from ba71fd6 to c8570f5 Compare November 28, 2023 08:05

olliw42 force-pushed the owpr-radiorcchannels branch from c8570f5 to 7c7f636 Compare November 28, 2023 20:16

tridge requested changes Nov 28, 2023

View reviewed changes

olliw42 mentioned this pull request Nov 29, 2023

RADIO_RC_CHANNELS message mavlink/mavlink#1919

Merged

olliw42 force-pushed the owpr-radiorcchannels branch from 7c7f636 to 20c1d70 Compare November 29, 2023 08:57

jlpoltrack mentioned this pull request Dec 6, 2023

development.xml: RADIO_RC_CHANNELS message ArduPilot/mavlink#343

Merged

peterbarker requested changes Dec 13, 2023

View reviewed changes

olliw42 added 3 commits December 13, 2023 18:08

RC_Channel: add DroneCAN, MAVRadio to RC_PROTOCOLS description

29f1e5b

GCS_MAVLink: handle RADIO_RC_CHANNELS

0d1b208

AP_RCProtocol: add support for MAVLink receiver, handle RADIO_RC_CHAN…

774203c

…NELS message

olliw42 force-pushed the owpr-radiorcchannels branch from 20c1d70 to 774203c Compare December 13, 2023 17:19

olliw42 mentioned this pull request Dec 28, 2023

Add MAVLink receiver support, RADIO_RC_CHANNELS, no II #25838

Closed

peterbarker requested changes Jan 24, 2024

View reviewed changes

auturgy added the DevCallEU label Jan 24, 2024

tridge removed the DevCallEU label Jan 24, 2024

olliw42 mentioned this pull request Feb 29, 2024

Add MAVLink receiver support, RADIO_RC_CHANNELS, no III #26356

Merged

olliw42 closed this Mar 1, 2024

olliw42 mentioned this pull request Mar 3, 2024

development.xml: RADIO_LINK statistics messages ArduPilot/mavlink#354

Open

Add MAVLink receiver support, RADIO_RC_CHANNELS #24577

Add MAVLink receiver support, RADIO_RC_CHANNELS #24577

Conversation

olliw42 commented Aug 10, 2023

magicrub commented Aug 11, 2023

olliw42 commented Aug 14, 2023 • edited Loading

olliw42 commented Nov 27, 2023

jlpoltrack commented Nov 27, 2023 • edited Loading

Ryanf55 Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

olliw42 Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ryanf55 left a comment

Choose a reason for hiding this comment

tridge left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olliw42 Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olliw42 Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbarker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olliw42 Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

tridge commented Nov 27, 2023

wvarty commented Nov 28, 2023

olliw42 commented Nov 28, 2023 • edited Loading

khancyr commented Nov 28, 2023

olliw42 commented Nov 28, 2023 • edited Loading

olliw42 commented Nov 28, 2023 • edited Loading

khancyr commented Nov 28, 2023

olliw42 commented Nov 28, 2023

tridge commented Nov 28, 2023

tridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olliw42 Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olliw42 commented Nov 29, 2023 • edited Loading

olliw42 commented Nov 30, 2023 • edited Loading

olliw42 commented Dec 3, 2023

olliw42 commented Dec 6, 2023 • edited Loading

olliw42 commented Dec 9, 2023 • edited Loading

olliw42 commented Dec 11, 2023

peterbarker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olliw42 commented Dec 13, 2023 • edited Loading

peterbarker commented Dec 26, 2023

olliw42 commented Dec 28, 2023 • edited Loading

auturgy commented Dec 28, 2023

olliw42 commented Jan 13, 2024

peterbarker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olliw42 commented Aug 14, 2023 •

edited

Loading

jlpoltrack commented Nov 27, 2023 •

edited

Loading

Ryanf55 Nov 27, 2023 •

edited

Loading

olliw42 Nov 28, 2023 •

edited

Loading

tridge left a comment •

edited

Loading

olliw42 Nov 28, 2023 •

edited

Loading

olliw42 Nov 28, 2023 •

edited

Loading

olliw42 Nov 28, 2023 •

edited

Loading

olliw42 commented Nov 28, 2023 •

edited

Loading

olliw42 commented Nov 28, 2023 •

edited

Loading

olliw42 commented Nov 28, 2023 •

edited

Loading

olliw42 Nov 29, 2023 •

edited

Loading

olliw42 commented Nov 29, 2023 •

edited

Loading

olliw42 commented Nov 30, 2023 •

edited

Loading

olliw42 commented Dec 6, 2023 •

edited

Loading

olliw42 commented Dec 9, 2023 •

edited

Loading

olliw42 commented Dec 13, 2023 •

edited

Loading

olliw42 commented Dec 28, 2023 •

edited

Loading