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

Converting library to C #328

Closed
Avamander opened this issue Jan 21, 2017 · 19 comments
Closed

Converting library to C #328

Avamander opened this issue Jan 21, 2017 · 19 comments

Comments

@Avamander
Copy link
Member

Avamander commented Jan 21, 2017

What do you think about converting this library to C?

I'm 100% sure this would both reduce the size of the end result and make the speed rely less on the compiler's optimizations (that fail way too often), in addition to that it would allow easier porting to some platforms that do not have C++ support.

@Avamander
Copy link
Member Author

Avamander commented Jan 21, 2017

@Oitzu @TMRh20 @mz-fuzzy and others what do you think of this?

@lcgamboa
Copy link

I already did this, take a look at:
https://github.com/lcgamboa/RF24_c
https://github.com/lcgamboa/RF24Network_c
https://github.com/lcgamboa/RF24Mesh_c
https://github.com/lcgamboa/RF24Ethernet_c
https://github.com/lcgamboa/RF24Gateway_c

I tested with PIC18F4620 and Arduino. The compiled code is small with avrgcc, but I don´t tested the speed. Only some arduino examples are converted to compatibility with new c library.

@Avamander
Copy link
Member Author

Avamander commented Jan 21, 2017

@lcgamboa Oh, that's pretty great. Though, have you thought about pushing your changes upstream? (Without renaming the library or making it specific to a single MCU)

@lcgamboa
Copy link

@Avamander I think that c++ is more suitable for arduino, and c is more portable. Without class/objects the c version API is not directly compatible with existing c++ code and examples.

Merge c++ version whith the c branch will result in no more compatibility with previous version, I don´t think so it´s a good idea, but I don´t no against it.

I renamed the library because the API incompatibility, for people don´t confuse with the c++ original version.

I have tested the c version with PIC and arduino, but it will work with any c compiller for microcontroller.

If the project maintainers are in favor of total library conversion to c version, I agree.

@Avamander
Copy link
Member Author

Avamander commented Jan 21, 2017

@lcgamboa A major version number bump, indicating an API change can be done. I doubt that will be much of an issue. The suitability won't be much different, though the syntax will change a bit. I think the work could be made in a new branch before merging to the master. (See https://github.com/TMRh20/RF24/tree/c-port, you can make the needed pushes/pull requests to that branch)

BTW it seems MySensors has converted the library to C too.

@henrikekblad
Copy link

Yes, we've slowly moved away from c++ classes in MySensors (but still have a couple left for convenience).

Like you said, it has been easier to port between archs and more importantly the footprint smaller.

@fedevo
Copy link

fedevo commented Jan 23, 2017

Hello maintainers,
I'm not an expert at all, and maybe my point makes no sense, but I do agree with @lcgamboa comments regarding impact on existing code and usage ...
I saw the pull request from @lcgamboa, hopefully in c-port branch, but it seems that the c++ version will disappear if it is merged into master !?
Many people uses the actual c++ version, how will you manage the transition and what will be the drawbacks of the C migration (ie: how to use multiple radios on the same board ?)
@Avamander is it not possible to maintain the 2 versions in parallel ?
regards,
Vincent

@lcgamboa
Copy link

Maintain two version in parallel require work in double.
The actual c version uses only one global object for radio because pass one object pointer in each fuction call increase the code size and decrease the speed in microcontrollers.
You can have an array of radios and change the copy of the specifc radio to global object before call every function to have support to multiple radios.
To matain the backward compatilbility with c++ is necessary create a c++ wrapper class to c functions, this is easy to be done, but the speed and code size will be worse than with actual c++ library.
What do you think about it?

@Avamander
Copy link
Member Author

Avamander commented Jan 23, 2017

The current idea is that the c-port branch is for the c port. The current pull request needs conflict resolving before I can merge it though (missing a few functions).

In addition to that:

  1. Code should be formatted according to the NASA C style guide
  2. Platform specific modifications need tidying
  3. Examples should be made universal (thus no specific platform examples)
  4. Printing to serial console also needs an unified library that supplies the necessary macros
  5. C++ compatibility API needs to be written (user can be #error-ed to convert and allow ignoring with some #define)
  6. Tests should be written so that in the future build server could be used to report any build errors easily.

About two radios, just like with RF24Network, multi-head functionality is a preprocessor flag, it can be implemented the same for RF24(c) too. Though with the C++ wrapper it should not be an issue.

@soligen2010
Copy link
Contributor

soligen2010 commented Jan 23, 2017

My take is that one way or another C++ support should be maintained for the Arduino users if nothing else. I think arduino is likely a large consumer base for this library. I am not an expert in C or C++, but I prefer C++ for the benefit of using objects. I don't know if it is possible, but if converted to C, can a C++ wrapper supporting the current interface be wrapped around it?

As far as footprint size goes, my take is that if I am that constrained on size, I would not use the library at all. For a size constrained system I may use the library for a first pass implementation to figure out what settings I needed, then when I needed the footprint to shrink, replace with my own streamlined code that talks directly to the device (keeping header files for the definitions).

@Avamander
Copy link
Member Author

Avamander commented Jan 24, 2017 via email

@fedevo
Copy link

fedevo commented Jan 24, 2017

As I'm a simple lib consumer, I'm not comfortable with asking again question but what I understand so far about the c++ to c migration is:
pro:

  • portability
  • performance ?
  • footprint ?

cons:

  • c style vs c++ style
  • c++ wrapper required to avoid impact on existing users (and so the RF24.h,cpp should not be renamed)
  • side effect and regression tests ?

question:

  • do you have some figure / comparison about performance and footprint ?
  • what is the case identified which requires the migration ?

Feel free to complete pro/cons/question sections ...
Thanks

@lcgamboa
Copy link

I renamed the library back to RF24 and I have made a c++ wrapper to c library (only for one radio). The c++ user don't need to change the code to use the c library with the c++ wrapper.
I have tested with the example pingpair_dyn.ino. The memory usage in bytes of this example compiled in arduino 1.6.12 are shown below:

  • Original c++ library: PROG 7626 RAM 318
  • c version: PROG 7342 RAM 331
  • c++ wrapper: PROG 7326 RAM 321

I don't have much time to work and test all examples for now, I appreciate any help.

@johnduhart
Copy link

Throwing my two cents in:

A major version number bump, indicating an API change can be done. I doubt that will be much of an issue.

I get what you're talking about (SemVer), but a major version bumps are meant to indicate breaking changes that require come effort to correct, not a complete rewrite of the API.

If you're going to go down this route, please fork the project.

@kaofishy
Copy link

There's really no reason why any equivalent C++ code would be bigger and slower than the C version. In fact in some instances you can generate smaller or faster code because certain C++ features allow for better inclining and ROM-ing (eg constexpr).

@lcgamboa
Copy link

There is reason, it depends on the compiler and the optimizations. A c ++ compiler is always much more complex in its construction than a c compiler, so its optimizations are more complex as well. In the case of avr-gcc and avr-g++, the code in c looks smaller than c ++.

@kaofishy
Copy link

If you look at the code sizes you provided, you see that the C++ wrapper ends up with smaller program and memory footprint than pure C, so essentially the abstractions cost nothing and may even allow for more optimization.

There's a great example of how much of modern C++ abstractions optimize to nothing. https://m.youtube.com/watch?v=zBkNBP00wJE

@kaofishy
Copy link

But I certainly understand that it would be nice for the library to be portable to platforms with limited or no c++ support, like 8051.

@lcgamboa
Copy link

I have converted to C the libraries RF24, RF24Network, RF24Mesh, RF24Ethernet and RF24Gateway for portability. Now this work for arduino and for any microcontroller with a C compiler. C++ compiler are not common between microcontrollers. For example using sdcc compiler the library now supports MCS51, DS80C390, HC08, Z80, STM8, PIC16 and PIC18 targets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants