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

Fixed adc config, to read internal reference voltage to get cpu temperature #13152

Merged
merged 6 commits into from
Mar 18, 2021

Conversation

dinomani
Copy link
Contributor

  1. Adc is reading the wrong channel for temperature on stm32f42xxxx, stm32f7. Channel should be 18.
  2. Also, it does not set the bit to enable and chose the according reference voltage. The register does not refer to the base of ADCx but to the ADCCommon register.
  3. The intended prescaler value of 2 is also not set because of the wrong adc base address.

Tested on fmuv5, reading channel 18 gives now plausible values.

@dagar
Copy link
Member

dagar commented Oct 14, 2019

Looks good. Do you have plans to expose this somewhere it can be logged?

@@ -260,6 +260,8 @@
#define ADC_HW_REV_SENSE_CHANNEL /* PC3 */ ADC1_CH(13)
#define ADC1_SPARE_1_CHANNEL /* PC4 */ ADC1_CH(14)

#define ADC_INTERNAL_TEMP_SENSOR_CHANNEL (18) /* No Gpio assigned, internal channel*/
Copy link
Member

Choose a reason for hiding this comment

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

nit - Comment is missing trailing space

I think we should look at putting this in <px4_platform/adc.h>
This is a chip dependent as to what ADC and what channel and is not not a board attribute.

F4:
Temperature sensor, VREFINT and VBAT internal channels
• For the STM32F40x and STM32F41x devices, the temperature sensor is internally
connected to channel ADC1_IN16.
The internal reference voltage VREFINT is connected to ADC1_IN17.
• For the STM32F42x and STM32F43x devices, the temperature sensor is internally
connected to ADC1_IN18 channel which is shared with VBAT. Only one conversion,
temperature sensor or VBAT, must be selected at a time. When the temperature sensor
and VBAT conversion are set simultaneously, only the VBAT conversion is performed.
The internal reference voltage VREFINT is connected to ADC1_IN17.
The VBAT channel (connected to channel ADC1_IN18) can also be converted as an injected
or regular channel.
Note: The temperature sensor, VREFINT and the VBAT channel are available only on the master
ADC1 peripheral
F7:
• The temperature sensor is internally connected to ADC1_IN18 channel which is shared
with VBAT. Only one conversion, temperature sensor or VBAT, must be selected at a
time. When the temperature sensor and VBAT conversion are set simultaneously, only
the VBAT conversion is performed.
The internal reference voltage VREFINT is connected to ADC1_IN17.
The VBAT channel is connected to ADC1_IN18 channel. It can also be converted as an
injected or regular channel.
H7:

The ADCs are connected to 5 internal analog inputs:
– the internal temperature sensor (VSENSE) is connected to ADC3 VINP/VINN[18]
– the internal reference voltage (VREFINT) is connected to ADC3 VINP/VINN[19]
– the VBAT monitoring channel (VBAT/4) is connected to ADC3 VINP/VINN[17]
– DAC internal channel 1, connected to ADC2 VINP/VINN[16]

@stale
Copy link

stale bot commented Jan 19, 2020

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Jan 19, 2020
@LorenzMeier
Copy link
Member

@dinomani @davids5 This should really be wrapped up.

@stale stale bot removed the stale label Mar 8, 2020
@stale
Copy link

stale bot commented Jun 6, 2020

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Jun 6, 2020
LorenzMeier
LorenzMeier previously approved these changes Jan 10, 2021
@stale stale bot removed the stale label Jan 10, 2021
@LorenzMeier LorenzMeier marked this pull request as ready for review January 10, 2021 17:44
@LorenzMeier
Copy link
Member

Needs a rebase.

@mrpollo
Copy link
Contributor

mrpollo commented Jan 20, 2021

Next steps

@MaEtUgR
Copy link
Member

MaEtUgR commented Jan 21, 2021

Rebased, thanks to @dinomani for the nice collaboration 👍

Copy link
Member

@davids5 davids5 left a comment

Choose a reason for hiding this comment

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

This is the composition: BOARD has CHIP, CHIP has ADC
Once the board select the chip the the ADC should be configured.
Since this is an attribute of the SoC it goes in the micro hal.

If it is in all micro hal's, then the Top of file #if !defined goes away.

@@ -237,7 +237,7 @@ uint32_t px4_arch_adc_temp_sensor_mask()
{
#ifdef ADC_INTERNAL_TEMP_SENSOR_CHANNEL
Copy link
Member

Choose a reason for hiding this comment

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

Please consider this:
Top of File

#if !defined(ADC_INTERNAL_TEMP_SENSOR_CHANNEL)
# define ADC_INTERNAL_TEMP_SENSOR_CHANNEL 16
#end

in code 
return 1 << ADC_INTERNAL_TEMP_SENSOR_CHANNEL;

In micro_hal for IC
#define ADC_INTERNAL_TEMP_SENSOR_CHANNEL nn

@dagar
Copy link
Member

dagar commented Jan 26, 2021

Can we get the data captured somewhere so it can actually be monitored? Either added to cpuload or a new message?

float32 ram_usage # RAM usage from 0 to 1

@LorenzMeier
Copy link
Member

@dinomani Could you please take the review comments into consideration? Let's wrap this up and get your fix in!

@MaEtUgR MaEtUgR force-pushed the temperature_sensor_fix branch from 438a41a to e84edd8 Compare February 10, 2021 13:04
@mrpollo
Copy link
Contributor

mrpollo commented Feb 24, 2021

@dinomani do you have any updates on this PR, do you have time to continue working on this feature or would you rather someone from the team help you take this to the finish line?

@dinomani dinomani force-pushed the temperature_sensor_fix branch from e84edd8 to 0192285 Compare March 10, 2021 09:48
@dinomani dinomani requested a review from davids5 March 10, 2021 09:50
Copy link
Member

@davids5 davids5 left a comment

Choose a reason for hiding this comment

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

@dinomani See in line - I am assuming the updates we discussed on the call today are coming. If not, let me know and I can can finish this.

@@ -65,5 +65,6 @@ int stm32h7_flash_lock(size_t addr);
int stm32h7_flash_unlock(size_t addr);
int stm32h7_flash_writeprotect(size_t block, bool enabled);
#define stm32_flash_lock() stm32h7_flash_lock(PX4_FLASH_BASE)
#define APX4_ADC_INTERNAL_TEMP_SENSOR_CHANNEL 17 //Valid for ADC3 on H7x3
Copy link
Member

Choose a reason for hiding this comment

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

Once this is corrected and the code to support it is added this should come in.

@dagar said add the temperature to top for the next PR. From there as a subcategory of system integrity like we discussed today.

Copy link
Member

Choose a reason for hiding this comment

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

I was talking about either ORB_ID(cpuload) or a new standalone topic (if there's more relevant data).

cpuload.load = interval_spent_time / interval;
#elif defined(__PX4_NUTTX)
// get ram usage
struct mallinfo mem = mallinfo();
cpuload.ram_usage = (float)mem.uordblks / mem.arena;
cpuload.load = 1.f - interval_idletime / interval;
#endif
cpuload.timestamp = hrt_absolute_time();
_cpuload_pub.publish(cpuload);

@@ -65,6 +65,8 @@ int stm32h7_flash_lock(size_t addr);
int stm32h7_flash_unlock(size_t addr);
int stm32h7_flash_writeprotect(size_t block, bool enabled);
#define stm32_flash_lock() stm32h7_flash_lock(PX4_FLASH_BASE)
#define APX4_ADC_INTERNAL_TEMP_SENSOR_CHANNEL 17 //Valid for ADC3 on H7x3
#define PX4_ADC_ADC3_CHANNEL_OFFSET 7
Copy link
Member

Choose a reason for hiding this comment

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

This is artificial. We should just compare for == PX4_ADC_INTERNAL_TEMP_SENSOR_CHANNEL

Copy link
Member

@davids5 davids5 left a comment

Choose a reason for hiding this comment

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

@dinomani - Thank you

one delete

davids5
davids5 previously approved these changes Mar 17, 2021
dinomani and others added 6 commits March 17, 2021 17:28
…channels were verified with ST_CUBE header files. For the H7 its a simplification for the current used H7x3. Other devices in that family expect the temperarture on channel 18.

F1: channel 16
F3: channel 16
F4: channel 18
F7: channel 18
H7: channel 17, on STMf32Hx3
Co-authored-by: David Sidrane <David.Sidrane@Nscdg.com>
@davids5 davids5 merged commit 4bf894a into PX4:master Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants