search

dronecan / DSDL

DSDL Protocol Description files for DroneCAN
MIT License
22 stars 60 forks source link

sensors: add RCInput message #26

Closed peterbarker closed 1 year ago

peterbarker commented 1 year ago

WIP; discuss before merge

David-OConnor commented 1 year ago

I would remove rssi and status; it's better to have it in a separate packet as:

Small point as you could ignore the field, but IMO it complicates things.

AlexKlimaj commented 1 year ago

Yeah a lower rate status packet would be fine. But it also is such a small difference in overall packet size having it in this message.

David-OConnor commented 1 year ago

Concur on the added bytes not being a big deal. My leaning against it is to prevent the question "if rssi here is 0, does that mean there is no rssi data? Or does it mean reference a link stats packet? What if the two disagree?"

You could answer those questions, but given a choice, I'd not ask them.

peterbarker commented 1 year ago

On Sat, 20 May 2023, Daniel Agar wrote:

@dagar commented on this pull request.

In dronecan/sensors/rc/20400.RCInput.uavcan:

@@ -0,0 +1,6 @@ +uint8 STATUS_RSSI_VALID = 1 # RSSI field is valid +uint8 STATUS_FAILSAFE = 2 # receiver has lost contact with transmitter +uint16 status + +uint8 rssi

percentage? How to handle the difference between not provided and zero?

First bit in the status field for rssi validity.

I've presumed percentage for RSSI but I'm open to suggestions.

dagar commented 1 year ago

I've presumed percentage for RSSI but I'm open to suggestions.

I'm not sure, we can't really win here because there are a lot different options. In newer systems like CRSF/ELRS it's actually in dBm and separately there's a link quality percentage (% of uncorrupted packets I believe).

Either way we should specify in a comment for the field so there's no ambiguity.

Another idea to keep in mind if the range of options becomes clear is that unions are possible in UAVCAN.

David-OConnor commented 1 year ago

What is the intent for 2-6 bit channels (ie switches)? This is concretely answered in FC implementations and doesn't necessarily affect this PR directly. Is the intent to pack into the 12-bit channels, or assign one per channel.

peterbarker commented 1 year ago

Concur on the added bytes not being a big deal. My leaning against it is to prevent the question "if rssi here is 0, does that mean there is no rssi data? Or does it mean reference a link stats packet? What if the two disagree?"

You could answer those questions, but given a choice, I'd not ask them.

RSSI may not be a valid field; that's what the status bit means. What 0 means when num_channels is non-zero is undefined. My current implementation won't send any DroneCAN packets when there's no RC input to the AP_Periph node, so it's not a problem there.

peterbarker commented 1 year ago

I've presumed percentage for RSSI but I'm open to suggestions.

I'm not sure, we can't really win here because there are a lot different options. In newer systems like CRSF/ELRS it's actually in dBm and separately there's a link quality percentage (% of uncorrupted packets I believe).

Either way we should specify in a comment for the field so there's no ambiguity.

Another idea to keep in mind if the range of options becomes clear is that unions are possible in UAVCAN.

I've added a comment in - 0-255

peterbarker commented 1 year ago

I've sprinkled some comments in, but if I don't hear some serious screaming we're probably going to call this done.

I think a link status packet in the future would be a good idea - but RSSI is sufficient for most purposes.

David-OConnor commented 1 year ago

Hi! Could someone please post the base CRC? Thank you! I have a prototype node (ELRS) set up to broadcast this message, and would like to confirm DroneCAN GUI can parse it. Ie, the output of this:

val = dronecan.DATATYPES[(20_400, 0)]
print(val.base_crc)

(I've been unable to successfully generate these myself, and use a map of PDC output)

peterbarker commented 1 year ago

I've moved this to a "more correct" ID.

I've increased the number of channels to 32 (@hendjoshsr71 )

I've renamed "rssi" to "quality" (@dagar )