Skip to content
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

add a new message about atmosphere #1646

Merged
merged 4 commits into from
Jul 29, 2021
Merged

Conversation

gitfishup
Copy link
Contributor

  1. Adding a new message about atmosphere data;
  2. The atmosphere data include temperature and humidity, they come from a hygrometer ,such as SHT3X sensor.
  3. The hygrometer communicate with flight(ardupilot and PX4) controller with uavcan protocol;
  4. The humidity and temperature can display on Mission Planner and QGroundcontroll.
  5. The hygrometer is use to detect other sensor's running status.

<description>hygrometer output atmosphere temperature humidity.</description>
<field type="uint32_t" name="time_boot_ms" units="ms">Timestamp (time since system boot).</field>
<field type="uint8_t" name="sensor_id">Sensor ID</field>
<field type="float" name="atmosphere_temp" units="degC">temperature</field>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be a float? I.e. what resolution can you get out of the sensor and is it meaningful to have more than a degree. What are max, min values you might reasonably encounter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be a float? I.e. what resolution can you get out of the sensor and is it meaningful to have more than a degree. What are max, min values you might reasonably encounter?

We need to keep two places behind the decimal point,for example , 35.66℃,65.35%。

Copy link
Contributor

@python36 python36 Jun 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The typical resolution for a temperature value is 0.01 degC, so the most suitable type for this field is int_16 with units "cdegC"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a unit called "cdegC"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Centi (symbol c) is a unit prefix in the metric system.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that's it.Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just call "temperature"

@auturgy
Copy link
Collaborator

auturgy commented Jun 9, 2021

A couple of things. Needs a better name: Perhaps just "HUMIDITY_SENSOR".
Also, is there a reason for an explicit time stamp? I don't imagine that humidity will change so fast, and the consumer of the data be so time dependent, that the inherited timestamp isn't good enough.
No issue with a message specifically for payload data, but we also need to discuss the use case - if this is for a specific vehicle model or vendor, it might not belong in common.

Copy link
Contributor Author

@gitfishup gitfishup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of things. Needs a better name: Perhaps just "HUMIDITY_SENSOR".
Also, is there a reason for an explicit time stamp? I don't imagine that humidity will change so fast, and the consumer of the data be so time dependent, that the inherited timestamp isn't good enough.
No issue with a message specifically for payload data, but we also need to discuss the use case - if this is for a specific vehicle model or vendor, it might not belong in common.

@auturgy @hamishwillee
The name : I think atmosphere can express temperature and humidity
time stamp: I also think the time stamp is unreasonable, but in order to improve the message, I added it, and maybe it can be removed
use case: This is use for all Plane model and also use in PX4 Autopilot and ardupilot.

Maybe we can go further disscuss, and i will change this message after discuss.

<description>hygrometer output atmosphere temperature humidity.</description>
<field type="uint32_t" name="time_boot_ms" units="ms">Timestamp (time since system boot).</field>
<field type="uint8_t" name="sensor_id">Sensor ID</field>
<field type="float" name="atmosphere_temp" units="degC">temperature</field>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be a float? I.e. what resolution can you get out of the sensor and is it meaningful to have more than a degree. What are max, min values you might reasonably encounter?

We need to keep two places behind the decimal point,for example , 35.66℃,65.35%。

@hamishwillee
Copy link
Collaborator

@gitfishup

"use case: This is use for all Plane model and also use in PX4 Autopilot and ardupilot."

That is not a use case, that is a statement. A use case would be something like. "The imu allows me to get a change in velocity in a particular direction. This allows me to estimate the position, and compare it to data from other sources like GPS and produce a better estimate of position blah blah.

So not "where you will use it", but why do we want a humidity sensor - what are you going to use it FOR?. Humidity is, for example a factor in pressure measurement. However we don't need it for that - since we get raw pressure with it factored in. So why?

@hamishwillee
Copy link
Collaborator

With respect to the timestamp, we don't just add fields for the fun of it since each has a cost to send over a poor channel. So we only include fields that matter. Now timestamp matters if the recipient needs to synchronize the data to other data. In this case it might be that no one cares about the time the data was sent - we can live with assumption that time it arrive is good enough.

This is one reason why we need to know your use case - without that we just have to guess it doesn't matter.

@hamishwillee
Copy link
Collaborator

We need to keep two places behind the decimal point,for example , 35.66℃,65.35%。

Why? I'm OK with this if the message was named HUMIDITY_SENSOR or HYGROMETER_SENSOR because it would mean "raw data" but if you are going to send bigger field in a message that is "generally" about atmosphere we kind of need to (again) know what it is actually useful for.

Sorry if this is not clear.

@gitfishup
Copy link
Contributor Author

We need to keep two places behind the decimal point,for example , 35.66℃,65.35%。

Why? I'm OK with this if the message was named HUMIDITY_SENSOR or HYGROMETER_SENSOR because it would mean "raw data" but if you are going to send bigger field in a message that is "generally" about atmosphere we kind of need to (again) know what it is actually useful for.

Sorry if this is not clear.

@hamishwillee Thanks for your reply.I'm sorry for not being clear. We've developed a new airspeed sensor that can be heated.The airspeed sensor heats up according to the set temperature, so as to prevent airspeed failure due to ice on the head of the airspeed sensor when the fixed-wing UAV is used in high and cold places.The temperature and humidity sensor is used to detect the temperature and humidity of the pitot tube head. If the measured temperature is lower than the set temperature, the airspeed sensor will enable heating to prevent ice. We can also use the humidity value to decide whether to heat or not.
Looking forward to your suggestions.

@python36
Copy link
Contributor

@hamishwillee Almost all humidity sensors also has a temperature sensor, so the temperature field must be in msg.

@gitfishup The atmosphere is not only characterized by temperature and humidity, so the name "HYGROMETER_SENSOR" is more appropriate.

@hamishwillee
Copy link
Collaborator

@gitfishup Thank you for the clarification - that is very helpful and enough for us to understand a bit more about the use case. Is the theory that the pilot would make the decision about heating, and that is why you need the information in the ground station. If so, doesn't that also mean that you will need a command to control your header elements etc?

If heating is automatic, I am assuming that this would just be "for information" - right?

My first thought is that HYGROMETER_SENSOR is better name. Not just because "atmosphere" covers more things, but because this is an unhelpful name at the times you're actually measuring airspeed sensor temperature.

You don't need a timestamp for this - if there was a delay of a second in your getting this then it would make no huge difference to the pilot. You should probably put an index value in the message - that allows multiple HYGROMETERS to be specified on the same vehicle - ie it is future proofing.

@auturgy Any comments?

@gitfishup
Copy link
Contributor Author

@gitfishup Thank you for the clarification - that is very helpful and enough for us to understand a bit more about the use case. Is the theory that the pilot would make the decision about heating, and that is why you need the information in the ground station. If so, doesn't that also mean that you will need a command to control your header elements etc?

If heating is automatic, I am assuming that this would just be "for information" - right?

My first thought is that HYGROMETER_SENSOR is better name. Not just because "atmosphere" covers more things, but because this is an unhelpful name at the times you're actually measuring airspeed sensor temperature.

You don't need a timestamp for this - if there was a delay of a second in your getting this then it would make no huge difference to the pilot. You should probably put an index value in the message - that allows multiple HYGROMETERS to be specified on the same vehicle - ie it is future proofing.

@auturgy Any comments?

@hamishwillee Thanks.
We currently set a temperature on the UAVCN GUI Tool or Mission Planner, and then the heating will be turned on. The real-time temperature can be observed on the ground station. If the temperature is not set enough during the flight, after landing,Use the UAVCN GUI Tool or Mission Planner to set the temperature higher.his would just be for information.

How about this index value?
<field type="uint8_t" name="id">Sensor ID (zero indexed). Used for multiple HYGROMETERS inputs</field>

@hamishwillee
Copy link
Collaborator

hamishwillee commented Jun 16, 2021

Thanks. So what is Mission planner sending to turn heating on?

Re

<field type="uint8_t" name="id">Sensor ID (zero indexed). Used for multiple HYGROMETERS inputs</field>

Normally we don't specify the index but by convention normally ids are "1" indexed (eg for batteries, imus etc). There is no particular reason not to use zero index or to specify it. Copying the contention of battery you would just do:

<field type="uint8_t" name="id" instance="true">Hygrometer ID</field>

FYI instance="true" tells recipients this same message is used for multiple sources, and identifies this field as the one that identifies each instance. This allows GCS to dynamically plot each instance rather than mixing up the data from different instances.

If I don't get feedback from other team members in this discussion I'll chase up in the dev call next week.

@gitfishup
Copy link
Contributor Author

Thanks. So what is Mission planner sending to turn heating on?

@hamishwillee Thanks a lot!
Just use ardupilot's SLCAN serial port to change the uavcan parameter,such as temperature, if the setting temperature greater than the temperature from hygrometer, the airspeed will turn heating on.

Re

<field type="uint8_t" name="id">Sensor ID (zero indexed). Used for multiple HYGROMETERS inputs</field>

Normally we don't specify the index but by convention normally ids are "1" indexed (eg for batteries, imus etc). There is no particular reason not to use zero index or to specify it. Copying the contention of battery you would just do:

<field type="uint8_t" name="id" instance="true">Hygrometer ID</field>

FYI instance="true" tells recipients this same message is used for multiple sources, and identifies this field as the one that identifies each instance. This allows GCS to dynamically plot each instance rather than mixing up the data from different instances.

If I don't get feedback from other team members in this discussion I'll chase up in the dev call next week.

Thanks again for your guidance and help!

@@ -82,5 +82,14 @@
<field type="uint16_t" name="data_rate" units="MiB/s">WiFi network data rate. Set to UINT16_MAX if data_rate information is not supplied.</field>
<field type="uint8_t" name="security" enum="WIFI_NETWORK_SECURITY">WiFi network security type.</field>
</message>
<message id="420" name="ATMOSPHERE_OUTPUT_STATUS">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change name to HYGROMENT_SENSOR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok , i will change it.

<wip/>
<!-- This message is work-in-progress and it can therefore change. It should NOT be used in stable production environments. -->
<description>hygrometer output atmosphere temperature humidity.</description>
<field type="uint32_t" name="time_boot_ms" units="ms">Timestamp (time since system boot).</field>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still don't know why this is needed

Copy link
Contributor Author

@gitfishup gitfishup Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean "Timestamp "?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hamishwillee I change name and datatype according to the disscussion.

@@ -82,5 +82,13 @@
<field type="uint16_t" name="data_rate" units="MiB/s">WiFi network data rate. Set to UINT16_MAX if data_rate information is not supplied.</field>
<field type="uint8_t" name="security" enum="WIFI_NETWORK_SECURITY">WiFi network security type.</field>
</message>
<message id="420" name="HYGROMENT_SENSOR">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo in name :-)

@hamishwillee
Copy link
Collaborator

@gitfishup Thanks very much for the update. FYI As a sensor message, I am/was OK with the greater resolution on temperature than whole degrees (i.e. did not have to be int8 allowing only resolution of 1 degree

We discussed this in the dev call. @auturgy is going to talk to you guys offline a bit more about this and will let us know if this does indeed meet our requirements.

@gitfishup
Copy link
Contributor Author

@gitfishup Thanks very much for the update. FYI As a sensor message, I am/was OK with the greater resolution on temperature than whole degrees (i.e. did not have to be int8 allowing only resolution of 1 degree

We discussed this in the dev call. @auturgy is going to talk to you guys offline a bit more about this and will let us know if this does indeed meet our requirements.

Thanks a lot .

@gitfishup
Copy link
Contributor Author

@auturgy @hamishwillee The new commit fixed typo in name

@gitfishup
Copy link
Contributor Author

@hamishwillee @auturgy Does this meet your requirements?

@hamishwillee
Copy link
Collaborator

The message looks OK to me. Originally you wanted a resolution of two decimal places on the temperature - is that not needed?

I'm waiting on @auturgy to confirm the result of his discussion.

@gitfishup
Copy link
Contributor Author

gitfishup commented Jul 14, 2021

resolution of two decimal places on the temperature

that is not needed.Thanks

@hamishwillee
Copy link
Collaborator

It may not be needed by this use case, and still be sensible for others.

@auturgy
Copy link
Collaborator

auturgy commented Jul 14, 2021

I'm in agreement with Hamish re units for degC - I think an int16 / cdegC is more consistent with temp fields elsewhere. Let's discuss on the call and either merge or modify and merge during the call.

<!-- This message is work-in-progress and it can therefore change. It should NOT be used in stable production environments. -->
<description>temperature and humidity from hygrometer.</description>
<field type="uint8_t" name="id" instance="true">Hygrometer ID</field>
<field type="int8_t" name="temperature" units="degC">temperature</field>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<field type="int8_t" name="temperature" units="degC">temperature</field>
<field type="int16_t" name="temperature" units="cdegC">temperature</field>

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@python36 For some reason I don't have edit/merge rights to PRs from your repo. Can you merge this? (and also give me rights to the repo so we can update the humidity if that is agreed?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@python36 Please merget this suggestion.

<description>temperature and humidity from hygrometer.</description>
<field type="uint8_t" name="id" instance="true">Hygrometer ID</field>
<field type="int8_t" name="temperature" units="degC">temperature</field>
<field type="uint8_t" name="humidity" units="%">humidity</field>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What precision can these sensors be read to?

Should we consider doing this with uint16_t and humidity*100?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typical accuracy of ±1.5%RH and ±0.2℃ for SHT35.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@python36 Can you please change this to uint16_t with units c% (that's centipercent = 100 times the humidity in %)

@gitfishup
Copy link
Contributor Author

I'm in agreement with Hamish re units for degC - I think an int16 / cdegC is more consistent with temp fields elsewhere. Let's discuss on the call and either merge or modify and merge during the call.

Thanks,hopefully it will merge.

@peterbarker
Copy link
Contributor

peterbarker commented Jul 14, 2021 via email

@hamishwillee
Copy link
Collaborator

@python36 We discussed this in the dev call last night. We're OK with hit provided the two fields are made double size with the units as indicated in comments. Once you make the changes I can merge (I don't have edit rights on your repo to do this)

@hamishwillee
Copy link
Collaborator

Ping. I'll merge when requested changes are made.

@gitfishup
Copy link
Contributor Author

Ping. I'll merge when requested changes are made.

@hamishwillee I have changed it ,please have a review. Thanks!

@hamishwillee
Copy link
Collaborator

Thanks very much! Merging.

@hamishwillee hamishwillee merged commit 2cc1a22 into mavlink:master Jul 29, 2021
@gitfishup
Copy link
Contributor Author

Thanks very much! Merging.

@hamishwillee Thanks for your help. Finally, i have a question, will this be merged into common.xml?

@hamishwillee
Copy link
Collaborator

Thanks for your help. Finally, i have a question, will this be merged into common.xml?

Once you have a working prototype that shows its value then you can create a PR to move it across. The point is that you create in development.xml, then it moves to common when we're all sure that any issues with the message have been worked out.

@gitfishup
Copy link
Contributor Author

Thanks for your help. Finally, i have a question, will this be merged into common.xml?

Once you have a working prototype that shows its value then you can create a PR to move it across. The point is that you create in development.xml, then it moves to common when we're all sure that any issues with the message have been worked out.

So that's it! I will do a work prototype and test it as soon as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants