-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
HIL message support for airflow sensor #1893
base: master
Are you sure you want to change the base?
Conversation
@@ -5723,6 +5723,12 @@ | |||
<field type="uint8_t" name="mode" enum="MAV_MODE_FLAG" display="bitmask">System mode. Includes arming state.</field> | |||
<field type="uint64_t" name="flags" display="bitmask">Flags as bitfield, 1: indicate simulation using lockstep.</field> | |||
</message> | |||
<message id="94" name="HIL_AIRFLOW_SENSOR"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have to be in the MAVLink 1 range? Usually better to put into MAVLink 2 range if at all possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason for it being in Mavlink 1 range. I just tried grouping the message near the HIL_* messages.
Is there any convention you follow when adding new messages?
@Jaeyoung-Lim @julianoes @dagar Does this make sense to you? |
<description>Simulated raw airflow measurements in SI units and relative to sensor datum</description> | ||
<field type="uint64_t" name="time_usec" units="us">Timestamp (UNIX Epoch time or time since system boot). The receiving end can infer timestamp format (since 1.1.1970 or since system boot) by checking for the magnitude of the number.</field> | ||
<field type="float" name="speed" units="m/s">Speed of airflow measureed through airflow sensor</field> | ||
<field type="float" name="direction" units="rad">Airflow direction measured relative to x-axis of sensor</field> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we make this more general, and and make this a three axis direction measurement?
<message id="94" name="HIL_AIRFLOW_SENSOR"> | ||
<description>Simulated raw airflow measurements in SI units and relative to sensor datum</description> | ||
<field type="uint64_t" name="time_usec" units="us">Timestamp (UNIX Epoch time or time since system boot). The receiving end can infer timestamp format (since 1.1.1970 or since system boot) by checking for the magnitude of the number.</field> | ||
<field type="float" name="speed" units="m/s">Speed of airflow measureed through airflow sensor</field> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Airflow sensors normally output differential pressure. Could we add a differential pressure field and maybe make the speed
and direction
fields optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW Sometimes easier if you make these suggestions with a concrete proposal that can be accepted :-).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks and agreed 👍 . Here is the Gazebo PR where I expand more. I should do the same for mavlink in the future.
Thanks @henrykotze @Jaeyoung-Lim My understanding is that simulation "generally" takes this set of messages https://docs.px4.io/main/en/simulation/#simulator-mavlink-api and that https://mavlink.io/en/messages/common.html#HIL_SENSOR would normally contain all sensor data.
Further, outside of HIL this kind of information is normally sent in SCALED_PRESSURE. Does this sensor do enough to justify extending that or adding its own message for sharing the information to a GCS? Upshot, I don't have enough context to understand if this is sensible or not. I need to understand the due diligence/thinking that has gone on. |
I would probably prefer an extensions to HIL_SENSOR if at all possible, so if it doesn't require too many fields. |
Yes, and perhaps make them zero default value if data not sent so that the message is efficient if the information is not supplied. |
Thanks all. To @hamishwillee first question: @Jaeyoung-Lim suggested that extending the HIL_SENSOR would break other HITL setups, that is why I moved away from this initial approach as mentioned here. For Context The sensor outputs the airflow speed and airflow direction as its raw measurement and nothing of differential pressure. We are also planning on using this sensor in our HIL setups. I am happy to proceed with your recommendation and hope this provides enough context for going forward. |
The way MAVLink works if you add an extension field:
The implication is that extension fields have to be designed such that a zero value means "this field is not being used or is invalid data". If the values are set to zero, they get truncated and there is no change in what data is sent before and after the extension. Upshot, this should not break HITL setups with proper field design. The only case that might cause problems is that if you do use the fields then more data would be sent and that might saturate a link. This would mostly be a consideration if the wind estimate is sent at a vastly different rate than the other data. @Jaeyoung-Lim Probably knows that, so will have to explain what he means if it is different.
For the "not HIL" message not sure what to do. Normally I'd say "SCALED_PRESSURE" is the airspeed sensor message, but that is not what you're sending. I THINK that an AIRSPEED_SENSOR message makes sense then systems can send which message they prefer. HOpe that helps. Will leave this open so @Jaeyoung-Lim can comment. |
I agree with @hamishwillee. Internally, we have made similiar changes to the HIL_SENSOR message a while back, and ran our HIL setups successfully, but as mentioned I am happy to proceed as recommended by the reviewers. |
FYI not forgotten this. Waiting on @Jaeyoung-Lim (who is a very busy man). I have pinged him off line and I hope to hear back in the next couple of weeks (hope you can wait!) |
@henrykotze Sorry for leaving you blocked by me not commenting @hamishwillee @julianoes IMHO, extending HIL messages are not the scalable solution, since it is not "just" one field that needs to be extended. This is partly why airspeed related simulations have been kept at a minimum(only differential pressure basically) and not really well for different failure modes (partial failures etc) The sensor @henrykotze is using is an ultrasonic wind sensor which measures airflow angles as well as airspeeds (not differential pressure). In our lab, we also internally have a wind vanes to measure sildeslip and angle of attack. I thought this new HIL message would be a nice way to start simulating these sensors in SITL/HITL. |
Thanks @Jaeyoung-Lim Not sure quite how to respond - didn't get to discussing this in the mavlink call. My understanding from the last discussion with @auturgy is that ArduPilot does not use the HIL messages for simulation integration, so their main interest in this is not cluttering up the namespace. From a MAVLink perspective my take is that we have a simulator API, so it should be fit for purpose. However I don't want to do this completely bit by bit. If you were designing this API right now, what would you add/change For this particular case, let's assume the fields are OK. This should go into development.xml at least until it is prototyped. The id of the message should be outside of the MAVLink 1 range (it can't be below 256). I will discuss this in the next MAV call if we can't get more useful opinions before then. |
@auturgy I will add this to discuss in next time dev call. Need to cover again what ArduPilot do for integration layer. IMO simulation APIs would be a good case to put in their own XML. Initially in common.xml, but perhaps selectable in a dialect - even "only included in POSIX or HIL builds". |
Yeah, that would be quite cool. |
Great. For now, you can move this back into development.xml, or wait until the conversation stabilises. Depending on how much you need it urgently. |
I'd suggest something like PX4_HIL.xml and get them out of common, to reduce confusion. |
I'd like to understand whether it is the case that a simulator API via MAVLink makes no sense (at all) such that no other flight stack would ever want to use these messages, or it is just the case that ArduPilot has gone another path. If it is the former then we might reasonably move these into Either way, separating out message sets makes it easier to talk about more "refined" support profiles. |
No description provided.