OBSTACLE_DISTANCE: add higher angle resolution and angle offset #1142
Conversation
|
@magicrub So before we had at most 72 elements with integer-spaced angles and starting with element 0 pointing north. I like it.
@mrivi @baumanta Any questions/thoughts/suggestions? Is there anyone else we know uses this message regularly and might offer useful feedback? |
message_definitions/v1.0/common.xml
Outdated
| @@ -5020,6 +5020,9 @@ | |||
| <field type="uint8_t" name="increment" units="deg">Angular width in degrees of each array element.</field> | |||
| <field type="uint16_t" name="min_distance" units="cm">Minimum distance the sensor can measure.</field> | |||
| <field type="uint16_t" name="max_distance" units="cm">Maximum distance the sensor can measure.</field> | |||
| <extensions/> | |||
| <field type="float" name="increment_f" units="deg">Angular width in degrees of each array element as a float. If >0 then this value is used instead of uint8_t increment.</field> | |||
| <field type="float" name="angle_offset" units="deg">Relative angle offset of first distance with index 0. Value of 0 corresponding to local North/forward and matches existing map and otherwise adds an offset. Positive is to the right.</field> | |||
There was a problem hiding this comment.
Maybe:
Relative angle offset of first distance with index 0. Value of 0 corresponding to local North/forward and matches existing map and otherwise adds an offset. Positive is to the right.
TO
Relative angle offset of the 0-index element in the distances array. Value of 0 corresponds to local North/forward. Positive values are offsets to the right.
- Note, not sure about "forward" in "North/forward" above. The old values only mention North.
- Removed "and matches existing map and otherwise adds an offset" because after this addition the "matching existing map" won't necessarily mean anything to a reader.
|
The advantage of a new packet would be to use a larger array at the end of the msg and allow 0 to be invalid. That would allow large msgs for high resolution and also short messages with the zero truncation on MAVLink 2. |
|
That being said, I think this packet extension is still useful. We can still add a new packet later on if we wish. |
|
Just to be pedantic, zero truncation starts with the smallest non-extension fields first, so that would only be a benefit if we could ensure that the other fields are at least the same size as items in the array. With current field sizes the array would not be truncated, and to make it so, we would have to increase all other fields to use uint16_t or greater. |
|
So, the benefit decreases. Even more of a reason to use the existing packet. |
That's not correct. It starts from the back of the payload, which will always be the extension fields in the order declared if they are present. So as soon as there was an extension to this that is used you can't benefit from trimming on the array. |
|
@WickedShell I'm talking about the new-packet scenario. I understand that the current packet will have an extension and will have non-zero data and this never shortened. However, it's a bit of a mute point because that array of 72 shouldn't ever be filled with 0 data anyway - should be filled with UINT16 |
|
@WickedShell As @magicrub said - I do fully under the truncation rules. Just clarifying if any benefit to a new message "in general", and in particular if the tradeoffs for truncation are worth it. You could still argue a new message is a cleaner approach but I don't think in this case. Other than my inline comments about the descriptions I like this. However I would like others who use the message to comment because I have no practical experience of the domain. If not before, I will make sure that happens on Dev Call. |
|
@hamishwillee @magicrub Sorry, I hadn't fully tracked the context on the previous messages, the comment about zero padding just caught my eye when skimming e-mail. I took it out of the context you meant, sorry about that! |
|
This seems to be an improvement for increments smaller than 5deg. I am okay with the change. |
|
@mrivi Thanks for the review. @hamishwillee does this really deserve to be a dev call topic? Seems minor. |
|
Not sure why Travis is failing, seems unrelated. Does anyone know about that? |
Yes, there is some formatting error - could be as simple as EOL being "wrong" or a space where not desired in message. You need to run |
Looks like a no-brainer improvement to me, but what do I know about the domain? Perhaps this extension might reduce the utility of further extensions in future. Perhaps someone in the dev call will have a strong reason to argue for a new message. So generally my take is that I try to get rubber stamp from the dev call, to ensure that at least some people with experience commented. @LorenzMeier @auturgy @WickedShell Speak now if you think this is a bad idea/have suggestions. Otherwise, if you really (?) need this urgently I will merge. |
|
well, the format_xml.sh script didn't do anything. I'm on Windows 10 but have git config set to LF. I just double checked and rebased and pushed the branch again to hopefully force a LF EOL. Meanwhile, since that particular EOL Travis error is totally bypassed by your git commands then it needs to be fixed in git by adding a .gitattributes file with an entry of
|
|
@magicrub I ran the format_xml (in a Linux vm) and the problem is the ">" in your descriptions. These need to be changed to Can you create the PR for gitattributes if still needed? (I can do this, but if someone questions the change, better it be defended by the person who has a deeper understanding of git than me) |
|
@hamishwillee I changed ">" to "greater than" annnnnnnnd now it passes. That's a pretty weird error that travis/BashScript should be a bit more clear on |
|
Thanks.
Yes and no. Yes, because it would be nice to get an error at this point so you understand for next time. No, because running the format script does indeed fix the problem without you having to know about it. Yes. All Travis does is run the format script, which in turn just runs the xmllint. So if you felt strongly you could create a PR that echos any issues it generates: https://github.com/mavlink/mavlink/blob/master/scripts/format_xml.sh#L37 |
|
Thanks @magicrub for your patience. Dev call agreed is good so merged. |
|
great, thank you for the help! |
This MAVLink v2 extension packet allows for flexible obstacle detection from multiple sensors of varying resolution while still being backwards compatible.
increment_f (to override increment):
When forced to send exactly 72 samples, having an integer spacing just didn't make sense. Needed to be float so that the correct angular range was understood
angle_offset:
Same as, and adds to, param "PRX_YAW_CORR" in ArduPilot. However, if you have two sensors this woudl now allow you to send multiple messages covering different areas (at varying resolutions). For example, a 360 degree spinning lidar + long-range forward radar/lidar + wide angle sonar(s).
With multiple messages, we can aggregate a better picture and down-scale the resolution to whatever the flight controller can handle. And that will improve as we get fancier flight controllers with more memory. Meanwhile, the data would no longer limited by the comms link, just on how to store it once received.