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

sys: add pseudomodule scanf_float #11511

Merged
merged 2 commits into from May 28, 2019

Conversation

@gdoffe
Copy link
Contributor

commented May 12, 2019

To read float number from stdin, add "-u scanf_float"
option to the linker.
This option is setup using a pseudomodule as it is already done for
printf_float.
Just add to your Makefile:
USEMODULE += scanf_float

Tested on STM32 and native arch.

@jcarrano

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

@gdoffe I PR'd unit tests against your banch: gdoffe#1

@jcarrano jcarrano added the Type: bug label May 13, 2019
@kaspar030

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

@jcarrano jcarrano added the Type: bug label 2 hours ago

Why is that a bug? The nanospecs disable features for a reason.

@jcarrano

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

@kaspar030 The bug is that there is a loss of functionality (a part of the libc is missing) and there is no way of bringing it back (until this PR, of course)

@jcarrano

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

What if scanf_float was made the default? It is part of the C standard library and it would be the least surprising thing to do to have it normally available - with the possibility to remove it if the ROM image becomes too big.

@kaspar030

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

What if scanf_float was made the default? It is part of the C standard library and it would be the least surprising thing to do to have it normally available - with the possibility to remove it if the ROM image becomes too big.

I don't think that argument scales to all the other C library functions that are not enabled by default. Some documentation about what is disabled might be very helpful, though.

@jcarrano

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

The argument cannot be extrapolated to those functions that require additional I/O functionality: datetime functions will only work if there is a RTC, file functions if there is a FS. In those cases, however, the standard specifies how to fail.

It has to be documented, yes, but users should refer to the documentation to learn how to do more advanced tricks (like disabling features to save flash) and not to learn how to make basic functionality work. That would seem more aligned with RIOT's goal of being easy to use by default.

@kaspar030

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

This PR's tests/minimal without USEMODULE += scanf_float:

   text    data     bss     dec     hex filename
   2980      16    2540    5536    15a0 /home/kaspar/src/riot.tmp/tests/minimal/bin/samr21-xpro/tests_minimal.elf

With ``ÙSEMODULE += scanf_float``` added to the makefile:

   text    data     bss     dec     hex filename
  21152     488    2548   24188    5e7c /home/kaspar/src/riot.tmp/tests/minimal/bin/samr21-xpro/tests_minimal.elf

That's just too expensive to enable by default. For float in general, the number one rule on embedded systems is "don't use at all".

@gdoffe

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

That's just too expensive to enable by default. For float in general, the number one rule on embedded systems is "don't use at all".

I agree it should not be activated by default. printf_float is not activated by default. But it needs to be documented somewhere (what is the best place for this ?).

We use it on a big STM32 (STM32F446) which have the capacity to use use it. However that cost of nearly 18kB seems unacceptable.

It is more a feature to activate if needed.

@jcarrano

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

That's just too expensive to enable by default.

Expensive? If it still fits into the flash, what's the problem then? If it is desirable to reduce the image size (say, because of flashing time at a production facility of for delivering via OTA) than that's an optimization issue- the user can figure out how to fine-tune his app.

I don't want to deviate the conversation too much from the main purpose of this PR (to make it possible to enable scanf_float). I may make a separate PR dealing with it being or not default.

@gdoffe

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

I don't want to deviate the conversation too much from the main purpose of this PR (to make it possible to enable scanf_float). I may make a separate PR dealing with it being or not default.

+1

@gdoffe

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Hi,

What is the next step for this PR ?

Thanks,
Gilles

@aabadie

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

What is the next step for this PR ?

Merge gdoffe#1 ? (use "rebase and merge" option). After that I have the impression that it will be good to go.

@gdoffe

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Thx @aabadie, done.

@aabadie

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

Now you need to rebase to drop the merge commit.

I let @jcarrano give a last round of review.

@gdoffe gdoffe force-pushed the gdoffe:scanf_float branch from 83592c3 to 1c82a75 May 23, 2019
gdoffe and others added 2 commits May 12, 2019
To read float number from stdin, add "-u scanf_float"
option to the linker.
This option is setup using a pseudomodule as it is already done for
printf_float.
Just add to your Makefile:
USEMODULE += scanf_float

Signed-off-by: Gilles DOFFE <g.doffe@gmail.com>
Newlib-nano does not seem to support hexadecimal floats or the %a
specifier. What is even weirder, it reports a successful conversion
anyways.

Tests for these two cases have been commented out.
@gdoffe gdoffe force-pushed the gdoffe:scanf_float branch from 1c82a75 to 66c66b5 May 28, 2019
@jcarrano jcarrano requested review from jcarrano and aabadie May 28, 2019
@jcarrano

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

@aabadie i'll approve it, but since the test was written by me it would be nice if somebody would give a second approval.

Copy link
Contributor

left a comment

ACK and go.

Thanks @gdoffe !

@aabadie aabadie merged commit e80fcca into RIOT-OS:master May 28, 2019
2 checks passed
2 checks passed
Murdock The build succeeded. runtime: 12m:59s
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gdoffe gdoffe deleted the gdoffe:scanf_float branch May 29, 2019
@zpw199062

This comment has been minimized.

Copy link

commented Jun 18, 2019

when i added USEMODULE += scanf_float in Makefile,and i entered the docker to build,it shows that

error: /home/zpw199062/backup/riot/RIOT-2019.04/tests/periph_gpio/bin/stm32f4discovery/scanf_float.a: No such file or directory
/home/zpw199062/backup/riot/RIOT-2019.04/tests/periph_gpio/../../Makefile.include:461: recipe for target '/home/zpw199062/backup/riot/RIOT-2019.04/tests/periph_gpio/bin/stm32f4discovery/tests_periph_gpio.elf' failed

how to solve it?
And my problem is also i cannot printf Float

@kaspar030

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

@zpw199062 I think "scanf_float" was merged after release 2019.04, which your folder name suggests you're using. You could try cherry-picking this commit: 7fc3207

@zpw199062

This comment has been minimized.

Copy link

commented Jun 18, 2019

@zpw199062 I think "scanf_float" was merged after release 2019.04, which your folder name suggests you're using. You could try cherry-picking this commit: 7fc3207

Thank you so much for the quikly reply, when i changed file pseudomodules.inc.mk, after i added the part you showed me,
then i opened it again, it shows that:
E325: ATTENTION
Found a swap file by the name "~/backup/riot/RIOT-2019.04/makefiles/.pseudomodul
es.inc.mk.swp"
owned by: zpw199062 dated: Tue Jun 18 02:03:20 2019
file name: ~zpw199062/backup/riot/RIOT-2019.04/makefiles/pseudomodules.
inc.mk
modified: YES
user name: zpw199062 host name: ubuntu
process ID: 7527
While opening file "/home/zpw199062/backup/riot/RIOT-2019.04/makefiles/pseudomod
ules.inc.mk"
dated: Mon Apr 29 04:28:52 2019

(1) Another program may be editing the same file. If this is the case,
be careful not to end up with two different instances of the same
file when making changes. Quit, or continue with caution.
(2) An edit session for this file crashed.
If this is the case, use ":recover" or "vim -r /home/zpw199062/backup/riot/R
IOT-2019.04/makefiles/pseudomodules.inc.mk"
to recover the changes (see ":help recovery").
If you did this already, delete the swap file "/home/zpw199062/backup/riot/R
IOT-2019.04/makefiles/.pseudomodules.inc.mk.swp"
to avoid this message.
-- More --
How to make sure that i already chaneg the file,pleas help me!

@kaspar030

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

The message just tells you what's wrong: probably you had the file still opened in another vim session.

@kaspar030

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

BTW, this is getting off-topic for this PR. @zpw199062 Could we maybe move to mailing list (devel@riot-os.org)?

@zpw199062

This comment has been minimized.

Copy link

commented on 7fc3207 Jun 19, 2019

Hi,gdoffe

Thank you first for the efforts solving this problem . And I tried to use the Riot-master version that you merged this "float problem" into, and i use it now. I do find that you changed the Make.include part and the pseudomodules.inc.mk which is added the float part. And i then tried the suggestion that adding USEMODULE += scanf_float in Makefile. It is built and flashed successfully, but when i run it. I still cannot see any float number. You can see the picture below.
image

And i want to show you my code for this part.
image
image

you can see that i defined the variable speed float type and try to print it ,but it still cannot work. Could you pls help me find the reason why?

Thanks so much!
Best regard!
By Peiwen

This comment has been minimized.

Copy link
Contributor

replied Jun 19, 2019

If you want to use float values with printf, you also need to add the printf_float module to your application.

This comment has been minimized.

Copy link

replied Jun 19, 2019

This comment has been minimized.

Copy link
Contributor

replied Jun 19, 2019

you mean we should use printf_float instead of print if we want to print float?

No, I said add USEMODULE += printf_float to your application the same you did with scanf_float module. There are already plenty applications doing this in RIOT codebase (try git grep printf_float). But in a word, use USEMODULE += scanf_float printf_floatand you'll be able to useprintfandscanf` with floating point values.

One last thing: as already pointed out by @kaspar030, don't comment in closed PRs for a usage question but rather send an email to devel@riot-os.org or users@riot-os.org mailnig lists.

This comment has been minimized.

Copy link

replied Jun 19, 2019

This comment has been minimized.

Copy link

replied Jun 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.