Skip to content

Add getTemperature() to support internal temp sensor #159

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

Merged
merged 2 commits into from
May 5, 2020

Conversation

nseidle
Copy link
Member

@nseidle nseidle commented Apr 29, 2020

This PR:

  • Adds a getTemperature() function. If there's a better func name, let me know. I did some googling for other platforms and didn't find any clear precident.

Function is wholly based on adc_vbatt.c example from latest Ambiq SDK. Function is required (rather than a ADC read and a series of maths) because there's a HAL call that reads the on board calibration factors. It was easier to leverage their call then roll our own.

uint32_t ui32Retval = am_hal_adc_control(g_ADCHandle, AM_HAL_ADC_REQ_TEMP_CELSIUS_GET, fVT);

getTemperature correctly pulls in user's chosen analogResolution() with a pow(2, _analogBits) calculation.

@stephenf7072
Copy link
Contributor

I've tried this branch, seems all good. getTemperature() returns believable values at room temperature and under hot air at nominally 100degC.

@oclyke
Copy link
Contributor

oclyke commented May 5, 2020

Code looks good! We're test-driving the new (more formal) process of merging PRs into the 'release-candidate' branch. @nseidle can you adjust this PR to target that branch?

I'm not sure if there is an easy way or if you will have to close this PR and open a new one targeting the correct branch. Let me know if you run into trouble since this is a pretty new process for us!

@nseidle nseidle force-pushed the adc_temperatureSensor branch from 4814dda to 870e83d Compare May 5, 2020 19:07
@nseidle nseidle changed the base branch from master to release-candidate May 5, 2020 19:09
@nseidle
Copy link
Member Author

nseidle commented May 5, 2020

Changed target branch (not too hard).

@oclyke
Copy link
Contributor

oclyke commented May 5, 2020

Awesome. I'm glad it was easy for you. I wonder if that had to do with your access level? PaulZC had trouble with the same task here

@oclyke oclyke self-assigned this May 5, 2020
Copy link
Contributor

@oclyke oclyke left a comment

Choose a reason for hiding this comment

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

While doing my final review I had one thought about the name of this function. Other than that it seems good and others report that it tests well (I loaned my hardware to Kyle last week and have yet to retrieve it)

@@ -44,6 +44,7 @@ ap3_err_t ap3_change_channel(ap3_gpio_pad_t padNumber);
bool power_adc_disable();
uint16_t analogRead(uint8_t pinNumber);
ap3_err_t analogReadResolution(uint8_t bits);
float getTemperature();
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like getTemperature() is just a bit too generic. What am I getting the temperature of? Should I be passing in an argument that is a value to convert to temperature?

How about something like getInternalTempC() or getChipTempC()?

@oclyke oclyke merged commit de4e8f8 into release-candidate May 5, 2020
@oclyke oclyke deleted the adc_temperatureSensor branch May 5, 2020 19:25
@nseidle
Copy link
Member Author

nseidle commented May 5, 2020

I'm all for a name change. getInternalTemp() is better IMO. Want me to make the changes?

@oclyke oclyke restored the adc_temperatureSensor branch May 5, 2020 20:50
@oclyke oclyke deleted the adc_temperatureSensor branch May 6, 2020 18:27
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.

3 participants