Temp sensor enable defines incorrect

[Hwurzburg]

Yuri reports that the enabling of the normal analog and MAX sensor is not enabled by default when temp sensor function enabled

Platform
[ x ] All
[ ] AntennaTracker
[ ] Copter
[ ] Plane
[ ] Rover
[ ] Submarine

[yuri-rage]

I added this issue, but Henry was faster at typing, so closed the duplicate. Here's the full text of my bug report:

AP_TEMPERATURE_SENSOR_ENABLED = 2 adds dummy support for temperature sensors but doesn't actually enable any drivers (including the basic backend). Even to get analog temperature sensor support, custom firmware must be built to explicitly set AP_TEMPERATURE_SENSOR_ENABLED = 1. Alternatively, using the Custom Firmware Builder, one has to check both Temperature Sensor support AND at least one of the I2C or SPI devices to get support in the custom build.

Our documentation states that we support up to 9 sensors, but a casual user would have to go to some length to discover why support doesn't actually exist in released firmware and figure out how to apply this workaround.

Version
4.5.5, 4.6-dev (minimum - issue likely predates these versions)

Platform
[ x ] All
[ ] AntennaTracker
[ ] Copter
[ ] Plane
[ ] Rover
[ ] Submarine

Airframe type
All

Hardware type
All boards with >2M flash (tested via CubeOrangePlus).

Logs
Shouldn't be required.

[peterbarker]

This issue doesn't actually say what you want to happen.

We can't change defaults on the custom build server - they're based on what's built into the firmware you're building.

This feels like it should be more documentation or interface tweaks than anything else.

What, exactly, do you think should happen?

[yuri-rage]

What, exactly, do you think should happen?

Apologies - I thought it was clear.

1. Temperature sensor support should be included by default on boards >2M.
2. Enabling "Temperature Sensors" on the custom firmware builder should AT LEAST enable the backend and analog sensors. At present, it's a red herring if you don't also select an I2C or SPI sensor in addition.

I initially made a PR for a big warning at the top of the Temp Sensor wiki page regarding lack of default support and the workaround via Custom Firmware Builder, but Henry argued that we should make the firmware match the expectation rather than documenting our way around it. That seems like the more logical choice to me, as well.

[Hwurzburg]

What, exactly, do you think should happen?

Apologies - I thought it was clear.

1. Temperature sensor support should be included by default on boards >2M.

2. Enabling "Temperature Sensors" on the custom firmware builder should AT LEAST enable the backend and analog sensors. At present, it's a red herring if you don't also select an I2C or SPI sensor in addition.

I initially made a PR for a big warning at the top of the Temp Sensor wiki page regarding lack of default support and the workaround via Custom Firmware Builder, but Henry argued that we should make the firmware match the expectation rather than documenting our way around it. That seems like the more logical choice to me, as well.

actually if the temp sensor function is enabled...analog, MAX, and DroneCan,if also present, sensors should be enabled...the intent is clear, but execution off...its also a regression:
#ifndef AP_TEMPERATURE_SENSOR_MAX31865_ENABLED
#define AP_TEMPERATURE_SENSOR_MAX31865_ENABLED AP_TEMPERATURE_SENSOR_BACKEND_DEFAULT_ENABLED
#endif

#ifndef AP_TEMPERATURE_SENSOR_ANALOG_ENABLED
#define AP_TEMPERATURE_SENSOR_ANALOG_ENABLED AP_TEMPERATURE_SENSOR_BACKEND_DEFAULT_ENABLED
#endif

#ifndef AP_TEMPERATURE_SENSOR_DRONECAN_ENABLED
#define AP_TEMPERATURE_SENSOR_DRONECAN_ENABLED AP_TEMPERATURE_SENSOR_BACKEND_DEFAULT_ENABLED && HAL_ENABLE_DRONECAN_DRIVERS
#endif

[yuri-rage]

To be clear the PR to add AP_TEMPERATURE_SENSOR_BACKEND_DEFAULT_ENABLED makes no impact to the issue at hand.

When the enable value is "2," the TEMPx params are not exposed to the user, which appears not to be the intent. And if the backend is actually present in this case, then it's a waste of flash. The fix should be to actually enable the feature on >2M boards, in line with the docs and apparent intent.

The issue exists both before and after Peter's PR.

[fadarnell]

Here's more info - hopefully helpful. After enabling "TEMP_MCP9600" and disabling "MODE_TURTLE" in this custom build for a Cube Orange, I was able to see the TEMPx_ params, but learned:

• I cannot access TEMPn beyond TEMP3
  Build: Cube Orange (and plus), Heli, Latest

I can help test if needed.

Some discussion here:
https://discuss.ardupilot.org/t/temp-params-missing-from-arduplane-4-3-7/105723/19

Edit: I learned the temperature related I2C addresses are limited to 0x60 to 0x66 and during testing I had tried 0x67. I have edited this comment to remove the following:
- the TEMPx_ADDR always resets to 96 no matter what address it is given

[magicrub]

Looks like I should dive into this driver and fix'r'up. Thanks for the bug reports!

[fadarnell]

I can confirm I've tested through TEMP3_ with success.

[magicrub]

I'm traveling and only have access to SITL right now. When I enable TEMP1_TYPE 2 I get a SITL I2C register read error crash. I've pinged @peterbarker to see if he can help me figure that out. It's unrelated to the main MCP9600 driver, just the SITL driver.. but still - the plot thickens!

[magicrub]

When setting type to 3 in SITL the params appear to work fine. I tested up to TEMP5.

[fadarnell]

This was about Type = 2

TEMP1_TYPE = 0:Disabled, 1:TSYS01, 2:MCP9600, 3:MAX31865, 4: TSYS03

Edit: at least for me it is. I can share my build from the build server

[magicrub]

@fadarnell I understand it's about type 2. Note, I tried type 2 on SITL and it crashed so I'm unable to test it right now... So I tried type 3 and the >3 limitation doesn't seem to be there. I'm testing in master. What version/hash are you testing on?

[fadarnell]

From the custom build server. build.log shows:
Vehicle: Heli
Board: CubeOrange
Remote: ardupilot
git-sha: 89c2b48
Version: latest-NA
Here's a zip of the build files.
drive-download-20241006T010124Z-001.zip

[peterbarker]

I'm traveling and only have access to SITL right now. When I enable TEMP1_TYPE 2 I get a SITL I2C register read error crash. I've pinged @peterbarker to see if he can help me figure that out. It's unrelated to the main MCP9600 driver, just the SITL driver.. but still - the plot thickens!

0d0ba0f (#24460) fixed a bug in the MCP9600 driver where we were reading 2 bytes instead of 1 for the WHOAMI register.

The simulator was not adjusted for this change, so would fail hard at startup. I've fixed the simulator for this.

#28324

The driver was also mucking around with a data-is-ready register bit which the simulator didn't support; I've improved its fidelity there. @magicrub I'm not sure the drivers is doing the right thing here - clearing that bit before reading the data out seems strange. I may be misunderstanding how that bit works, but that seems racey.

I turned on logging and changed the simulator to just return a constant 26.5 degrees and checked that is logged successfully:

I have also changed this to enable temperature sensors on all boards with more than 2MB of flash. That includes SITL but does not include the vast majority of STM32 boards (CubeRed and a few others).

We do not have flash space to include this on 2MB boards by default.

[CraigElder]

Resolved here
#28402