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

tests: net: lib: lwm2m: fix the missing float support #62431

Merged
merged 1 commit into from
Sep 11, 2023

Conversation

SgrrZhf
Copy link
Member

@SgrrZhf SgrrZhf commented Sep 8, 2023

The tests content_json and content_plain_test depend on the float support of libc. After PR##57340, Picolibc would be selected in these two tests and the PICOLIBC_IO_FLOAT won't be selected if the platform doesn't select FPU.

This commit select CONFIG_PICOLIBC and CONFIG_PICOLIBC_IO_FLOAT for these two tests.

Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

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

Hmm, if picolib would be used by default, then we should not need the CONFIG_PICOLIBC=y or did I misunderstood something?
I am not convinced that we should select config options like this, what if user running the test does not want to use picolib.

The tests `content_json` and `content_plain_test` depend on the float
support of libc. After PR#zephyrproject-rtos#57340, Picolibc would be selected in
these two tests and the `PICOLIBC_IO_FLOAT` won't be selected if
the platform doesn't select `FPU`.

This commit select `CONFIG_PICOLIBC` and `CONFIG_PICOLIBC_IO_FLOAT`
for these two tests.

Signed-off-by: Huifeng Zhang <Huifeng.Zhang@arm.com>
@SgrrZhf
Copy link
Member Author

SgrrZhf commented Sep 11, 2023

if picolib would be used by default, then we should not need the CONFIG_PICOLIBC=y

Sure, I'll delete it.

@SeppoTakalo
Copy link
Collaborator

Well, if tests are broken because of default configs, should we fix the default config instead of modifying the test configuration?

I noticed also another problem with Picolib #62444
So I think we need to consider whether LwM2M should require Newlib, or some other C library. Or should we just ignore some of the text-formatting issues?
After all, I don't think any realistic use case would use clear text, or JSON, based formats in product. It is just for development and debugging phase.

@jukkar
Copy link
Member

jukkar commented Sep 11, 2023

So I think we need to consider whether LwM2M should require Newlib, or some other C library. Or should we just ignore some of the text-formatting issues?

IMHO, for tests the most important thing is that it (the test) verifies that some functionality works ok. So with this in mind, the textual output of the test might not matter that much. Unless we should be verifying that the output of some print is correct, but I think that is not the case here.

For some other components than tests, I think it depends how important is it that the output is valid. I have encountered this few years go with gPTP and the net-shell where some 64-bit values were not printed correctly with minimal libc. I decided to ignore the issue as the printing was fixed eventually and the output was not that important for the user.

@SeppoTakalo
Copy link
Collaborator

SeppoTakalo commented Sep 11, 2023

So with this in mind, the textual output of the test might not matter that much. Unless we should be verifying that the output of some print is correct, but I think that is not the case here.

In these tests, we are in fact verifying the output of string formatting functions:

double value[] = { 0., 0.123, -0.987, 3., -10., 2.333, -123.125 };
char * const expected_payload[] = {
"0.0", "0.123", "-0.987", "3.0", "-10.0", "2.333", "-123.125"
};

So there is a known floating point value, and known text output that it should produce.
Without the config, the textual content format are completely unable to produce floating point values and integers bigger than 32 bit.
Whether this is a problem or not, is up to discussion. I'm starting to lean towards ignoring this issue as long as it is documented. I would leave it up to developer to choose if they need clear text formats, or should they use binary ones.

SeppoTakalo
SeppoTakalo previously approved these changes Sep 11, 2023
@nashif nashif merged commit 662a14a into zephyrproject-rtos:main Sep 11, 2023
15 checks passed
@SgrrZhf SgrrZhf deleted the missing-float-support branch September 12, 2023 06:55
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.

7 participants