Skip to content

Conversation

@FRASTM
Copy link
Contributor

@FRASTM FRASTM commented Jan 30, 2024

Fixing this "revealed" bug which got introduced when a PR added the -Wdouble-promotion flag to GCC builds
Based on the recent PR #68237

Fixes #68284

@FRASTM FRASTM marked this pull request as ready for review January 30, 2024 11:12
@FRASTM FRASTM requested a review from avisconti as a code owner January 30, 2024 11:12
@zephyrbot zephyrbot added the area: Sensors Sensors label Jan 30, 2024
Fixing this "revealed" bug which got introduced when a PR
added the -Wdouble-promotion flag to GCC builds

Signed-off-by: Francois Ramu <francois.ramu@st.com>
nordicjm
nordicjm previously approved these changes Jan 30, 2024
Copy link
Contributor

@yperess yperess left a comment

Choose a reason for hiding this comment

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

Is this really what we want? Shouldn't the fix be to change SENSI_GRAIN_XL to a float instead of a double by adding f to the constant:

#define SENSI_GRAIN_XL (61 / 1000.0f)

I think it's better than to force using doubles here.

@avisconti
Copy link
Contributor

Is this really what we want? Shouldn't the fix be to change SENSI_GRAIN_XL to a float instead of a double by adding f to the constant:

#define SENSI_GRAIN_XL (61 / 1000.0f)

I think it's better than to force using doubles here.

I tend to agree.

@FRASTM
Copy link
Contributor Author

FRASTM commented Feb 1, 2024

Is this really what we want? Shouldn't the fix be to change SENSI_GRAIN_XL to a float instead of a double by adding f to the constant:

#define SENSI_GRAIN_XL (61 / 1000.0f)

I think it's better than to force using doubles here.

I tend to agree.

Sure but I do not see this is enough. Warning messages remain

./samples/sensor/lsm6dsl/src/main.c:150:59: warning: implicit conversion from 'float' to 'double' when passing argument to function [-Wdouble-promotion]
  150 |                                                           out_ev(&accel_x_out),

@avisconti
Copy link
Contributor

I approved here, as it is the simplest solution. Nevertheless I guess that all that sensitivity values there could be expressed as ug/LSB using just a uint32_t. Obviously all the "GRAINS" definition should be expressed in ug/LSB as well (e.g. #define SENSI_GRAIN_XL (61LL)) and the conversion to mg (or mdps and so on) should be done in the correspondent get_channel() routine. What do you think?

@erwango
Copy link
Member

erwango commented Feb 1, 2024

@yperess The problem is that this warning shows up un CI in other PRs and hence is blocking them. I suggest approving this one and raising an issue for improvement (or a bug as you wish).
This can be fixed later on. Issue was there initially anyway..

I know that this is exactly why warning are there, but point is now detected. It doesn't have to be blocking.

@yperess
Copy link
Contributor

yperess commented Feb 1, 2024

Is this really what we want? Shouldn't the fix be to change SENSI_GRAIN_XL to a float instead of a double by adding f to the constant:

#define SENSI_GRAIN_XL (61 / 1000.0f)

I think it's better than to force using doubles here.

I tend to agree.

Sure but I do not see this is enough. Warning messages remain

./samples/sensor/lsm6dsl/src/main.c:150:59: warning: implicit conversion from 'float' to 'double' when passing argument to function [-Wdouble-promotion]
  150 |                                                           out_ev(&accel_x_out),

I'm sorry, I need a little more detail, out_ev is defined:

static inline float out_ev(struct sensor_value *val)
{
	return (val->val1 + (float)val->val2 / 1000000);
}

In this case val1 and val2 are int32_t types. I don't see how this PR solves this issue in samples/sensor/lsm6dsl/src/main.c unless it's trying to convert 1000000 to a double in which case you can just add .0f.

In my experience when we cover up these warnings, nobody ever goes back and fixes them. I would really prefer to just see a proper solution instead of moving to doubles which are much more costly.

@avisconti
Copy link
Contributor

avisconti commented Feb 5, 2024

@erwango @FRASTM @yperess
I created the PR #68549 which does not touch the driver and should remove the warning. Would you please take a look at it? It may replace this PR.

@henrikbrixandersen
Copy link
Member

Can this be closed now that #68549 was merged?

@avisconti
Copy link
Contributor

Can this be closed now that #68549 was merged?

I guess so. It seems to me that the blocked PR have eventually been merged

@erwango
Copy link
Member

erwango commented Feb 8, 2024

Closing

@erwango erwango closed this Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

compilation warning -Wdouble-promotion on samples/sensor/lsm6dsl

9 participants