Skip to content

minimal libc: add strtoll and strtoull functions #36645

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

Closed
mglettig opened this issue Jun 30, 2021 · 10 comments · Fixed by #39612
Closed

minimal libc: add strtoll and strtoull functions #36645

mglettig opened this issue Jun 30, 2021 · 10 comments · Fixed by #39612
Assignees
Labels
area: Minimal libc Minimal C Standard Library Enhancement Changes/Updates/Additions to existing features

Comments

@mglettig
Copy link

mglettig commented Jun 30, 2021

Is your enhancement proposal related to a problem? Please describe.
I'm using a Nordic NRF9160 chip. This supports AT_PARSER a lib which servers the built-in cellular modem using AT commands. Due to that fact that this lib needs strtoll and strtoull they added a dependency to newlibc. This dependency is new with the latest SDK that has a new version of AT_PARSER. Using newlibc is not possible for me due to incompatibility with Civetweb.

Describe the solution you'd like
I would be happy if somebody could add strtoll and strtoull functions to the minimal libc.

Describe alternatives you've considered
I tried to switch to newlibc which didn't succeed due to a incompatibility with Civetweb.

Additional context
Discussion in Nordic Forum
#16511

@mglettig mglettig added the Enhancement Changes/Updates/Additions to existing features label Jun 30, 2021
@henrikbrixandersen henrikbrixandersen added the area: Minimal libc Minimal C Standard Library label Jul 1, 2021
@stephanosio
Copy link
Member

I tried to switch to newlibc which didn't succeed due to a incompatibility with Civetweb.

@mglettig That would require some investigation, but do you think it would be feasible for you to use newlib if Civetweb and/or newlib are updated such that they can be used together?

The minimal C library, as its name suggests, is supposed to be "minimal", and we should refrain from adding more functions to it just to satisfy the ever-growing demand for more features, until it effectively becomes newlib-nano.

Moreover, you would probably want to use the newlib and take advantage of all the features available in a full C library anyway if you are working with a complex application that includes a web server. Noting that most SoCs that can handle Civetweb (including nRF9160) are not highly resource-constrained, I think the best approach here is to update the relevant components such that Civetweb can be used with newlib.

@manoj153
Copy link
Contributor

manoj153 commented Jul 19, 2021

The minimal C library, as its name suggests, is supposed to be "minimal", and we should refrain from adding more functions to it just to satisfy the ever-growing demand for more features, until it effectively becomes newlib-nano.

hi @stephanosio

have used newlibc yet strtoull() doesn't return expected unsigned 64bit return, wondering where is it messing up?

this is the snippet working on my 64bit intel, ubuntu machine but not with a stm32 mcu:

int main()
{
    char *ptr;
    char *my_str = "E0040150C8DF749D\n";
    
    uint64_t test = 0;
    test = strtoull(my_str, &ptr, 16);
    printf(" %llu", test);

 

    return 0;
}
 

@stephanosio
Copy link
Member

have used newlibc yet strtoull() doesn't return expected unsigned 64bit return, wondering where is it messing up?

@manoj153 Could you be more specific? i.e. how doesn't it work? example code? etc.

@manoj153
Copy link
Contributor

have used newlibc yet strtoull() doesn't return expected unsigned 64bit return, wondering where is it messing up?

@manoj153 Could you be more specific? i.e. how doesn't it work? example code? etc.

provided the snippet code above, my bad forget to include expected value, and what actually i got:

expected (correct) value : 16142028410881799325
return value : 18446744073709551615

@stephanosio
Copy link
Member

@manoj153 Tested that on qemu_cortex_m3 and it seems to be working fine -- not really sure what the problem is in your case. If you think this is a Zephyr-related bug, please file a bug report with the details and we can have further investigation there.

@mglettig
Copy link
Author

@mglettig That would require some investigation, but do you think it would be feasible for you to use newlib if Civetweb and/or newlib are updated such that they can be used together?

@stephanosio Yes that would be feasible for us. Would you have time to make Civetweb compatible to newlibc? I'm not sure where the incompatibility comes from but I remember that there was a discussion about this in one of the GitHub issues.

@stephanosio
Copy link
Member

Yes that would be feasible for us. Would you have time to make Civetweb compatible to newlibc?

Not at the moment because I am occupied with other urgent matters, but I will try once I get those sorted out.

I'm not sure where the incompatibility comes from but I remember that there was a discussion about this in one of the GitHub issues.

#16683 suggests that using Newlib alongside Civetweb is not an option "due to heavy conflicts with the POSIX layer."

In general, in line with #13787, I plan to implement and upstream newlib-side Zephyr RTOS support in the future (see zephyrproject-rtos/sdk-ng#350), so that might provide a solution to this problem eventually.

cc @Nukersson @pfalcon @carlescufi

@pfalcon
Copy link
Collaborator

pfalcon commented Jul 19, 2021

#16683 suggests that using Newlib alongside Civetweb is not an option "due to heavy conflicts with the POSIX layer."

I personally never agreed with that evaluation. There's a bunch of small conflicts, yeah. And resolving them is cumbersome, because moving one part may make other break. But eventually it all should converge to a stable state, and I did a bunch of tweaks in that direction previous years.

In general, in line with #13787, I plan to implement and upstream newlib-side Zephyr RTOS support in the future (see zephyrproject-rtos/sdk-ng#350), so that might provide a solution to this problem eventually.

Sounds good.

@KozhinovAlexander
Copy link
Collaborator

It would be very nice, if you can implement it. I hope I can find a time to support with reviews and testing.

@manoj153
Copy link
Contributor

@manoj153 Tested that on qemu_cortex_m3 and it seems to be working fine -- not really sure what the problem is in your case. If you think this is a Zephyr-related bug, please file a bug report with the details and we can have further investigation there.

my million apologies, there is something done wrong my end earlier, with the latest test, that snippet, is working as intended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Minimal libc Minimal C Standard Library Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants