Skip to content

Conversation

@daneos
Copy link
Contributor

@daneos daneos commented Jul 19, 2022

Update temperature API to return temperature with greater precision as float.
This addresses this comment in #7.

@CLAassistant
Copy link

CLAassistant commented Jul 19, 2022

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link

Memory usage change @ 3865633

Board flash % RAM for global variables %
arduino:mbed_nano:nanorp2040connect 🔺 0 - +2220 0.0 - +0.01 0 - 0 0.0 - 0.0
Click for full report table
Board examples/SimpleAccelerometer
flash
% examples/SimpleAccelerometer
RAM for global variables
% examples/SimpleGyroscope
flash
% examples/SimpleGyroscope
RAM for global variables
% examples/SimpleTemperature
flash
% examples/SimpleTemperature
RAM for global variables
%
arduino:mbed_nano:nanorp2040connect 0 0.0 0 0.0 0 0.0 0 0.0 2220 0.01 0 0.0
Click for full report CSV
Board,examples/SimpleAccelerometer<br>flash,%,examples/SimpleAccelerometer<br>RAM for global variables,%,examples/SimpleGyroscope<br>flash,%,examples/SimpleGyroscope<br>RAM for global variables,%,examples/SimpleTemperature<br>flash,%,examples/SimpleTemperature<br>RAM for global variables,%
arduino:mbed_nano:nanorp2040connect,0,0.0,0,0.0,0,0.0,0,0.0,2220,0.01,0,0.0

@per1234 per1234 added type: enhancement Proposed improvement topic: code Related to content of the project itself labels Jul 20, 2022
Copy link
Contributor

@aentinger aentinger left a comment

Choose a reason for hiding this comment

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

Hi ☕ 👋 You are not obeying the API contract with this change, i..e people who have passed an int to readTemperature will be in for a surprise. Please add another function, i.e. readTemperatureFloat to retrieve the float value (and may it only be a cast to the result of the other function).

@daneos
Copy link
Contributor Author

daneos commented Aug 22, 2022

I added the requested changes.

I have to admit though, that I was definitely more surprised to learn it was reading the temperature as int.
My reasoning is the temperature is usually read as float, the device is perfectly capable of reading it with resolution fit for a float value and the documentation says The values returned are signed floats.

Is there a reason for it to be integer?

@daneos daneos requested a review from aentinger August 22, 2022 12:34
@aentinger
Copy link
Contributor

I agree the API could have returned a float from the start. The discussion point for me is not what is the better API, but rather how can we honour the API contract established by preceding releases of this library. Now, with an additional function we are not breaking code that is already in use.

Copy link
Contributor

@aentinger aentinger left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@aentinger aentinger merged commit ea374f7 into arduino-libraries:master Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

topic: code Related to content of the project itself type: enhancement Proposed improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants