add support for sprintf

[dstroy0]

now you can use your RF24 object to format a buffer with debugging information

[2bndy5]

Well, this came out of nowhere. I tried this a while back, but got stumped on what I thought was reference problems. I have questions:

1. Have you tested this? If so, on what boards/platforms?
2. What are the code-size implications? Does this implementation add to the compile size? We have mainly used printf() on ATMega328P and tried to patch it with Arduino cores that support printf(). I'm not worried about Linux in this regard.
3. Specifically, what is this targeting? I don't know all the Arduino cores that support sprintf() off the top of my head.

[dstroy0]

1. I'm using it on ESP32/ESP8266 it should be ok for anything with more than 2k of free ram. The strlen on my implementation is 869,
2. the formatting string literal adds to flash memory, I thought about adding a directive toggle for the new members but I wasn't sure if I should do that, I'm new to using github and to contributing.
3. I wanted to be able to send the object over mqtt and I'm also able to print it out over the free version of webserial now
4. I'm learning how to use github and I'll work on the other stuff you asked about once I figure out how, probably tomorrow

[dstroy0]

I tested it on the teensy 4.1 (ARM Cortex-M7), I'll try a mega2560r3 tomorrow, and I think I have an m4 if you want to try more nxp.

[2bndy5]

Ran the CI tests, and this breaks all builds for the Pico SDK and most builds for Linux. Fortunately, I think its an easy fix: You need to add #include <stdio.h> to the RF24.cpp file. This is a common gotcha for people coming from Arduino land because #include <Arduino.h> brings a lot of other header files.

If for some reason adding #include <stdio.h> breaks Arduino builds, then the line needs to be added to the RF24_config.h file (for any platform that needs it). I can't seem to trigger the CI builds for Arduino (which tests various boards provided by the Arduino IDE) & PlatformIO (which tests Teensy boards). Usually, a commit like this would trigger them, but I don't know what is going on.

I thought about adding a directive toggle for the new members but I wasn't sure if I should do that

Typically we only do this when a feature cannot/shouldn't be supported by a certain platform. So, if sprintf() isn't supported by a certain chip (like the ATTiny), then we'd need to do this.

[dstroy0]

I tested on the mega2560 r3, working.

[2bndy5]

I'm more concerned as to why the CI workflows aren't getting triggered. This makes testing the changes on various boards and platforms super easy (without having to hookup a physical board and everything). I'm thinking I need to close and re-open this PR to trigger the CI... We need to make sure the #include <stdio.h> fixed the builds for the Pico SDK and Linux.

[2bndy5]

We tend to use doxygen primarily for documentation generation from src code comments. If you run doxygen at the root folder of this repo, then point your internet browser to the RF24/docs/html/classRF24.html file you should be able to see how the new function is documented.

I say all of this because the new function needs a:

@param debugging_information The c-string buffer that the debugging information is stored to.

You could word the description differently and elaborate how you like, but the parameter isn't documented for void RF24::sprintfPrettyDetails(char * debugging_information)

[dstroy0]

I'm more concerned as to why the CI workflows aren't getting triggered. This makes testing the changes on various boards and platforms super easy (without having to hookup a physical board and everything). I'm thinking I need to close and re-open this PR to trigger the CI... We need to make sure the #include <stdio.h> fixed the builds for the Pico SDK and Linux.

I don't know what CI workflows are, I've got a lot to learn about contributing and collaborating. AVR has their own stdio.h, I don't know if sprintf_P is standard C++. If it's not there are other ways I can get around that.

[2bndy5]

CI stands for Continuous Integration. Github has docs about using them, and the results tend to be in the repo's "Actions" tab at the top of the page.

sprintf() is part the C standard lib stdio.h, and I believe it should also define the macro sprintf_P in that header file. If sprintf_P is specific to Arduino cores, then I think we can work around it by

#ifndef sprintf_P
    #define sprintf_P sprintf
#endif

in the bottom of RF24_config.h

[2bndy5]

Just built this locally on ubuntu, and it turns out that we need

#include <stdio.h>

#ifndef sprintf_P
    #define sprintf_P sprintf
#endif

I'll have to get the pico SDK updated for my machine, and check that fixes the Pico SDK builds as well...

[2bndy5]

Ok, the #ifndef sprintf_P fix works for the Pico SDK builds as well.

BTW, it took almost 15 minutes to build the RPi Pico examples for 1 board. And that took place after the 5-10 minutes to setup the Pico SDK for my machine. The CI workflow builds the Pico examples for all supported boards in less than 5 minutes; this is another reason I'm concerned with the CI workflows not getting triggered.

EDIT: I'm actually working from Windows using the WSL Ubuntu terminal, so some of the delay could be attributed to that tactic.

[2bndy5]

My second review: This is looking better. We are now down to fixing English stuff (& a syntax typo)

[2bndy5]

Congrats @dstroy0 ! I can now accept this change into master. Here is what the new function's docs look like on readthedocs,org.

@Avamander @TMRh20 Any objections/comments?

[2bndy5]

I fixed the indentation (manually) and trimmed the trailing whitespaces (a feature in vscode).

about the python wrapper

Currently, this addition is not present in the python wrapper, and I added this fact to the function's docs.

I don't think that this function would be easy to add to the python wrapper as there would have to be a conversion from c-string obj to python unicoded string obj implemented when calling a wrapped/modified version of sprintfPrettyDetails() from the python interpreter. Using boost.python makes this more difficult than using pyind11. I suppose we can cross that bridge if someone makes a case for it...

[dstroy0]

I need to get better with python so I'll give it a try, I've never used the python wrapper.

[2bndy5]

Its awesome you're willing to try! Here's some pointers:

1. source code for the python wrapper is written in C/C++ and can be found in RF24/pyRF24/pyRF24.cpp. So, you don't actually need to learn python syntax until you go to test out your newly wrapped function within a python interpreter/REPL.
2. We use boost.python to help wrap the RF24 class into a python interface. It might help to understand the build process (which is outlined in the docs). The hardest part about this is reading the boost.python docs and the pydocs about creating C-extensions for CPython.
3. The way I envision this in python would be like so:

   from RF24 import RF24
   radio = RF24(22, 0)
   if not radio.begin():
       print("failed to init SPI")
       exit(1)
   
   # debug_info is a str() that is returned from the wrapper
   debug_info = radio.sprintfPrettyDetails()
   print(debug_info)

   This way we don't have to ¹take in a python str as an arg, ²convert it to a c-string to assemble the info, and ³convert it back to a python string when returning the buffer.
   We would simply ¹create a c-string with a wrapped version of the new function, ²pass that c-string to the function you wrote already, and ³return that c-string after converting it to a python string.

[2bndy5]

This CPython function might make your life easier.

[2bndy5]

I decided not to port this feature to Circuitpython_nRF24L01 lib because ¹memory on Circuitpython running that lib is already being stretched thin for networking API and ²there is already enough exposed API (including private members - because its pure python) for users to manage their own implementation of this feature.

@dstroy0 Mind if I try hacking this feature into the python wrapper or are you still cooking up another surprise?

[dstroy0]

I’m still reading the boost python docs. Should I keep this pull request open? I’m curious to see the solution. I’ve got vscode up and running, getting my feet wet using it. Is there anything else that needs attention?

…

On Jan 14, 2022, at 2:29 PM, Brendan ***@***.***> wrote:  I decided not to port this feature to Circuitpython_nRF24L01 lib because 1memory on Circuitpython running that lib is already being stretched thin for networking API and 2there is already enough exposed API (including private members - because its pure python) for users to manage their own implementation of this feature. @dstroy0 Mind if I take try to hack this feature into the python wrapper or are you still cooking up another surprise? — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you were mentioned.

[2bndy5]

Its not unusual for a PR to remain open while the feature it introduces gets ironed out. Aside from settling what to do with the python wrapper, I don't think anything else needs attention. I'll try coding what I've got stuck in my head and see if it builds. If it builds I'll commit it and test it on RPi...

Words of the wisedom

• Most new features are hypothetically discussed in an issue before submitting a PR. This is the stage of this PR currently because no related issue preceded it.
• When submitting a PR, it usually originates from a non-main/non-master branch of the fork. Github has docs about typical contributing workflows. This can also be done in vscode instead of using github
  1. click the branch icon
     
  2. select an existing branch or create a new one
     

[2bndy5]

Sometimes I surprise myself when I get it right the first time!

@dstroy0 This commit works to add the new function to the python wrapper. Result from testing on my RPi4:

>>> from RF24 import RF24
>>> radio = RF24(22, 0)
>>> radio.begin()
True
>>> s = radio.sprintfPrettyDetails()
>>> print(s)
================ SPI Configuration ================
CSN Pin                 = 0
CE Pin                  = 22
SPI Frequency           = 10 Mhz
================ NRF Configuration ================
Channel                 = 76 (~ 2476 MHz)
RF Data Rate            = 1 MBPS
RF Power Amplifier      = PA_MAX
RF Low Noise Amplifier  = Enabled
CRC Length              = 16 bits
Address Length          = 5 bytes
Static Payload Length   = 32 bytes
Auto Retry Delay        = 1500 microseconds
Auto Retry Attempts     = 15 maximum
Packets lost on
    current channel     = 0
Retry attempts made for
    last transmission   = 0
Multicast               = Disabled
Custom ACK Payload      = Disabled
Dynamic Payloads        = Disabled
Auto Acknowledgment     = Enabled
Primary Mode            = TX
TX address              = 0x65646f4e32
pipe 0 (  open  ) bound = 0x65646f4e32
pipe 1 (  open  ) bound = 0x65646f4e31
pipe 2 ( closed ) bound = 0xc3
pipe 3 ( closed ) bound = 0xc4
pipe 4 ( closed ) bound = 0xc5
pipe 5 ( closed ) bound = 0xc6


>>>repr(s)
"'================ SPI Configuration ================\\nCSN Pin\\t\\t\\t= 0\\nCE Pin\\t\\t\\t= 22\\nSPI Frequency\\t\\t= 10 Mhz\\n================ NRF Configuration ================\\nChannel\\t\\t\\t= 76 (~ 2476 MHz)\\nRF Data Rate\\t\\t= 1 MBPS\\nRF Power Amplifier\\t= PA_MAX\\nRF Low Noise Amplifier\\t= Enabled\\nCRC Length\\t\\t= 16 bits\\nAddress Length\\t\\t= 5 bytes\\nStatic Payload Length\\t= 32 bytes\\nAuto Retry Delay\\t= 1500 microseconds\\nAuto Retry Attempts\\t= 15 maximum\\nPackets lost on\\n    current channel\\t= 0\\r\\nRetry attempts made for\\n    last transmission\\t= 0\\r\\nMulticast\\t\\t= Disabled\\nCustom ACK Payload\\t= Disabled\\nDynamic Payloads\\t= Disabled\\nAuto Acknowledgment\\t= Enabled\\nPrimary Mode\\t\\t= TX\\nTX address\\t\\t= 0x65646f4e32\\npipe 0 (  open  ) bound\\t= 0x65646f4e32\\npipe 1 (  open  ) bound\\t= 0x65646f4e31\\npipe 2 ( closed ) bound\\t= 0xc3\\npipe 3 ( closed ) bound\\t= 0xc4\\npipe 4 ( closed ) bound\\t= 0xc5\\npipe 5 ( closed ) bound\\t= 0xc6\\n\\n'"

However, notice that there is extra whitespace at the end of the returned string; executing repr(s) shows that the string ends with \n\n. While any call to print() will append a \n automatically, it seems that the returned value already ends in a \n. Is this expected behavior for arduino? I think that the user can control the last line's termination by using Serial.println() vs Serial.print()

[dstroy0]

It’s not necessary, you are correct the user should specify spacing after. Just the one new line makes it like the output of printPrettyDetails. Any padding you want to remove to make the output smaller is ok with me. The function local buffer sizes can be optimized too so that memory use for the function goes down.

…

On Jan 17, 2022, at 2:11 PM, Brendan ***@***.***> wrote:  @2bndy5 commented on this pull request. In RF24.cpp: > + "\n" + "pipe 3 ( " + PRIPSTR + " ) bound\t= 0x" + PRIPSTR + "\n" + "pipe 4 ( " + PRIPSTR + " ) bound\t= 0x" + PRIPSTR + "\n" + "pipe 5 ( " + PRIPSTR + " ) bound\t= 0x" + PRIPSTR + "\n\n" @dstroy0 If your scenario is ok with these changes, then I'll commit them. I'm not sure if the \n\n is required for sending the c-string over MQTT, but I think it can still be added on the user's end not the lib's end. — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you were mentioned.

[2bndy5]

I'm ready to merge this now.
python wrapper test:

>>> s = radio.sprintfPrettyDetails()
>>> print(s)
================ SPI Configuration ================
CSN Pin                 = 0
CE Pin                  = 22
SPI Frequency           = 10 Mhz
================ NRF Configuration ================
Channel                 = 76 (~ 2476 MHz)
RF Data Rate            = 1 MBPS
RF Power Amplifier      = PA_MAX
RF Low Noise Amplifier  = Enabled
CRC Length              = 16 bits
Address Length          = 5 bytes
Static Payload Length   = 32 bytes
Auto Retry Delay        = 1500 microseconds
Auto Retry Attempts     = 15 maximum
Packets lost on
    current channel     = 0
Retry attempts made for
    last transmission   = 0
Multicast               = Disabled
Custom ACK Payload      = Disabled
Dynamic Payloads        = Disabled
Auto Acknowledgment     = Enabled
Primary Mode            = TX
TX address              = 0x65646f4e32
pipe 0 ( open ) bound   = 0x65646f4e32
pipe 1 ( open ) bound   = 0x65646f4e31
pipe 2 (closed) bound   = 0xc3
pipe 3 (closed) bound   = 0xc4
pipe 4 (closed) bound   = 0xc5
pipe 5 (closed) bound   = 0xc6
>>> len(s)
856

Any more optimizations can be submitted via another PR. I just want to get this upstream because it may well support boards that can't use printDetails() due to lack of printf() support.

[2bndy5]

well the CI workflows finally got triggered...

@dstroy0 You can see that this change does not harm builds for any of the officially supported boards in the Arduino CI run. The PlatformIO CI run shows all green as well (for all Teensy boards). The Linux CI run is looking good too.

Keep in mind that these workflows only test compile all the examples, and none of the examples actually call sprintfPrettyDetails(). So, it may not be the best approach for code coverage, but at least we know that the next release won't be completely broken.

[dstroy0]

Excellent!

…

On Jan 17, 2022, at 4:48 PM, Brendan ***@***.***> wrote:  I'm ready to merge this now. python wrapper test: >>> s = radio.sprintfPrettyDetails() >>> print(s) ================ SPI Configuration ================ CSN Pin = 0 CE Pin = 22 SPI Frequency = 10 Mhz ================ NRF Configuration ================ Channel = 76 (~ 2476 MHz) RF Data Rate = 1 MBPS RF Power Amplifier = PA_MAX RF Low Noise Amplifier = Enabled CRC Length = 16 bits Address Length = 5 bytes Static Payload Length = 32 bytes Auto Retry Delay = 1500 microseconds Auto Retry Attempts = 15 maximum Packets lost on current channel = 0 Retry attempts made for last transmission = 0 Multicast = Disabled Custom ACK Payload = Disabled Dynamic Payloads = Disabled Auto Acknowledgment = Enabled Primary Mode = TX TX address = 0x65646f4e32 pipe 0 ( open ) bound = 0x65646f4e32 pipe 1 ( open ) bound = 0x65646f4e31 pipe 2 (closed) bound = 0xc3 pipe 3 (closed) bound = 0xc4 pipe 4 (closed) bound = 0xc5 pipe 5 (closed) bound = 0xc6 >>> len(s) 856 Any more optimizations can be submitted via another PR. I just want to get this upstream because it may well support boards that can't use printDetails() due to lack of printf() support. — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you were mentioned.