adafruit / Adafruit_nRF52_Arduino

Adafruit code for the Nordic nRF52 BLE SoC on Arduino
Other
606 stars 492 forks source link

Add SAADC offset calibration and reading of CPU temperature #683

Closed ajs123 closed 3 years ago

ajs123 commented 3 years ago

These additions

Notes:

hathach commented 3 years ago
  • readCPUTemperature() returns a floating point value. Internally, the TEMP peripheral provides an integer with 1 LSB = 0.25 degrees C. Repeatability is +/- 0.25 C but accuracy is only +/- 5 C. The floating point value is a convenience for the novice user. If just passing along the integer is preferred, I can change it.

It is perfect choice, I am happy with float. M4 has FPU after all.

  • Though initiating offset calibration is probably the main reason for checking the temperature, readCPUTemperature() was added to wiring.c/h rather than to wiringanalog. I'd be happy to move it if another location is preferred. I didn't see any more logical place, and it didn't seem worthwhile to create any new files.

Perfect location

  • Though existing header files include #defined values for flag and trigger fields, both functions used hard-coded constants of 0x00 for not ready or disable, nonzero for ready, and 0x01 for triggers. This follows the existing pattern as well as many Nordic examples, so it is likely to be reasonably future-proof.

no problem at all, I use direct 0,1 for enable as well.

  • I left in a few comments that perhaps should be removed.

the more comments, the better it is

ajs123 commented 3 years ago

Oops! Sorry about that. I can do it tonight or in the morning.

On Thu, Aug 5, 2021 at 2:53 PM Ha Thach @.***> wrote:

@.**** commented on this pull request.

Thank you very much for your PR. Everything look great and spot on, I only have a minor request to add void explicitly to function signature. If you are busy, I will add it later on, not a problem at all.

In cores/nRF5/wiring_analog_nRF52.c https://github.com/adafruit/Adafruit_nRF52_Arduino/pull/683#discussion_r683705993 :

@@ -284,6 +284,25 @@ uint32_t analogReadVDD( void ) return analogRead_internal(SAADC_CH_PSELP_PSELP_VDD); }

+analogCalibrateOffset()

would you mind adding the return type and argument explicitly to void

void analogCalibrateOffset(void)


In cores/nRF5/wiring.c https://github.com/adafruit/Adafruit_nRF52_Arduino/pull/683#discussion_r683706484 :

@@ -143,3 +143,27 @@ void systemOff(uint32_t pin, uint8_t wake_logic) NRF_POWER->SYSTEMOFF = 1; } } + + +float readCPUTemperature()

float is perfect, it is indeed the right choice for the function. Though can you add void as its explicit argument

float readCPUTemperature(void)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/adafruit/Adafruit_nRF52_Arduino/pull/683#pullrequestreview-723689024, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABC4G2KGIDR6DTVVD42DYM3T3LM4LANCNFSM5BQ65CZA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .