Clean up temperature msg fields #24272
Open
+40
−58
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
haumarco
force-pushed
the
pr_clean_temperature_usage
branch
from
e60a339 to
695bfb9
Compare
🔎 FLASH Analysispx4_fmu-v5x [Total VM Diff: -112 byte (-0.01 %)]px4_fmu-v6x [Total VM Diff: -120 byte (-0.01 %)]Updated: 2025-02-03T10:06:38 |
sfuhrer
reviewed
Jan 30, 2025
| { | ||
| vehicle_air_data_s air_data; | ||
|
|
||
| if (_vehicle_air_data_sub.update(&air_data)) { |
There was a problem hiding this comment.
This will need a small edit of the comment in the MAVLink message.
"Air temperature from airspeed sensor" is no longer valid.
There was a problem hiding this comment.
There was a problem hiding this comment.
@sfuhrer It doesn't work that way - if this PR merges PX4 is no longer compliant with the MAVLink specification. The message itself is fine :-)
Whether we can update the message in MAVLink depends on whether there is a semantic difference that matters to consumers. I don't know, but that requires agreement with other stakeholders.
There was a problem hiding this comment.
I know, and we'll update the wording on the mavlink RP. Never the less, we're convinced that this is the right approach.
@haumarco to be compliant with the mavlink message we can for now also subscribe to differential_pressure.temperature here.
sfuhrer
reviewed
Feb 4, 2025
| float32 baro_pressure_pa # Absolute pressure in Pascals | ||
| float32 ambient_temperature # Abient temperature in degrees Celsius |
There was a problem hiding this comment.
Should we add a flag for the source of this field? Differential pressure, external baro, default ambient air temperature?
| @@ -193,7 +193,7 @@ class MavlinkStreamHighresIMU : public MavlinkStream | |||
| msg.abs_pressure = air_data.baro_pressure_pa; | |||
| msg.diff_pressure = differential_pressure.differential_pressure_pa; | |||
| msg.pressure_alt = air_data.baro_alt_meter; | |||
| msg.temperature = air_data.baro_temp_celcius; | |||
| msg.temperature = air_data.ambient_temperature; | |||
There was a problem hiding this comment.
Shouldn't that field be filled with the IMU temperature? How about subscribing to vehicleImuStatus.msg/temperature_accel or _gyro?
| vehicle_air_data_s air_data; | ||
|
|
||
| if (_vehicle_air_data_sub.update(&air_data)) { | ||
| _temperature.add_value(air_data.ambient_temperature, _update_rate_filtered); |
There was a problem hiding this comment.
Should we only publish a value if it's actually coming a sensor and is not the hard coded 15°C?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Solved Problem
Currently, multiple temperature measurements are used across different functionalities to represent ambient temperature. This creates ambiguity regarding which temperature measurement should be designated as
vehicle_temperature. We lack dedicated temperature sensors or topics to consistently rely on a single source.Solution
Implement a prioritized selection of temperature sensors. When an external airspeed sensor is available, its temperature is used as the primary source. If an external barometer is present, its temperature serves as the secondary option. The temperature from the internal barometer is disregarded due to its unreliability, which is caused by inconsistent PCB heat dissipation across different flight controllers.
Changelog Entry
Create a distinct ambient_temperature field in the VehicleAirData message.Alternatives
An alternative approach would be to create a dedicated temperature uORB topic to centralize ambient temperature data.
Test Coverage
Tested with various setups and hardcoded measurements in simulation environments to ensure reliability and accuracy.