nRF24 / RF24

OSI Layer 2 driver for nRF24L01 on Arduino & Raspberry Pi/Linux Devices
https://nrf24.github.io/RF24
GNU General Public License v2.0
2.21k stars 1.02k forks source link

printDetails() do not work on SAMD platform (Adafruit Feather M0) #414

Closed DanoPTT closed 3 years ago

DanoPTT commented 6 years ago

It looks that there is problem with printf function used in printDetails(), when library is used with Adafruit Feather M0 board (same processor like on Zero boards, identified as SAMD_ZERO Cortex M0). Probably it is due the remapping Serial to USBSerial. I have found out that Serial.println works, but printf doesn't.

2bndy5 commented 3 years ago

+1 to this. I tried the same thing using both Arduino IDE & PlatformIO using suggested code from docs, and still printDetails() doesn't work.

TMRh20 commented 3 years ago

Yeah, the printf.h file only includes support for AVR and X86 devices. Most other devices seem to have it working by default.

It sounds like printf is not working at all on SAMD? I'm guessing Serial.printf doesn't work either?

Looking here: https://github.com/arduino/ArduinoCore-samd/pull/458 it looks like a modification would need to be made to get it working. If the Arduino API and SAMD platform are not going to include it, changes could maybe be made to the printf.h file to make it work with SAMD, but I don't have a device to test with.

2bndy5 commented 3 years ago

Serial.printf() does work on my Adafruit Feather M0 Express!

If the Arduino API and SAMD platform are not going to include it, changes could maybe be made to the printf.h file to make it work with SAMD

I'm ok with closing this and just deferring to the status of #189 & #322 because the issue you mentioned referenced the fact that the

Arduino team has specifically decided against including printf...

I don't mind trying to alter printf.h for this issue if it helps progress the other 2 issues I referenced. But I wouldn't impose upon everyone (with this particular problem) take that action.

TMRh20 commented 3 years ago

You might be able to jus do:

define printf Serial.printf

On Sep 17, 2020, at 3:40 PM, Brendan notifications@github.com wrote:

 Serial.printf() does work on my Adafruit Feather M0 Express!

If the Arduino API and SAMD platform are not going to include it, changes could maybe be made to the printf.h file to make it work with SAMD

I'm ok with closing this and just deferring to the status of #189 & #322 because the issue you mentioned referenced the fact that the

Arduino team has specifically decided against including printf...

I don't mind trying to alter printf.h for this issue if it helps progress the other 2 issues I referenced. But I wouldn't impose upon everyone (with this particular problem) take that action.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

2bndy5 commented 3 years ago

clever idea, but it doesn't seem to work. I put the #define printf Serial.printf before #include "RF24.h". I don't understand why it doesn't work. Calling Serial.printf() & printf() (with the clever define) does work, but not for printDetails().

TMRh20 commented 3 years ago

https://github.com/nRF24/RF24/blob/master/RF24_config.h#L155

If you take a look at the above, I'm going to guess it has to do with the program memory related defines. It may be necessary to redefine a few more things for SAMD to use normal functions instead of progmem related ones.

ie: change

        #ifndef printf_P
          #define printf_P printf
        #endif

to

          #define printf_P printf

@2bndy5 You seem to have a very good grasp of programming along with an understanding of RF24, would you like to be an 'official' contributor? You could make small changes as required, but for larger changes, it would be good to do a PR.

2bndy5 commented 3 years ago

would you like to be an 'official' contributor?

YES PLEASE! I mean, it would be my honor. Of course, I would open a simple issue (if 1 doesn't exist) for your (& other's) input before making small commits. I absolutely agree about the PR stipulation (github can be a nightmare for experts). Not sure if/when the awe for your work on this device will "wear off". My nRF24 understanding comes from my pure python attempt to mimic this repo for CircuitPython devices, but the ATSAMD51 has very little memory left for user-space code (~15kB) with the CircuitPython firmware loaded. So, I've been recommending this library for boards with that chip in relation the nRF24L01.

Back to the issue at hand. I have to dig deeper into the SAMD Arduino core to see what other changes might need to be made. Initial attempt with example you posted wasn't enough.

TMRh20 commented 3 years ago

Awesome, I mostly just want to keep up to date with changes, so for urgent fixes or really minor changes, feel free to do what you need. Things can always be reverted.

ps: I'm sure it will wear off quickly once you realize that none of us really know what we're doing :p

2bndy5 commented 3 years ago

Follow up to the comment I made on that issue: DEAD END found the post with a link to a zip supposedly containing working code on the STM32 concerning printf... Unfortunately, the forum must have suffered an attack or meltdown. the link goes to the forum home page with a notice at the top talking about restoring data.

2bndy5 commented 3 years ago

Figured I'd look into this again (since I seem to be on a roll sifting through the #ifdef soup). And I got it working on my adafruit QtPy board (uses the same exact ATSAMD21 as the adafruit feather M0).

For those interested I just added to the RF24_arch.h using the ARDUINO_ARCH_SAMD macro :

    #else // !defined (ARDUINO) || defined (ESP_PLATFORM) || defined (__arm__) || defined (__ARDUINO_X86__) && !defined (XMEGA)
        #if !defined (ARDUINO) // This doesn't work on Arduino DUE
            typedef char const char;

        #elif defined (ARDUINO_ARCH_SAMD)
            #define printf Serial.printf

        #else // Fill in pgm_read_byte that is used, but missing from DUE
            #if defined (ARDUINO_ARCH_AVR) || defined (ARDUINO_ARCH_SAMD)
                #include <avr/pgmspace.h>
// ...

changes will be part of #750

2bndy5 commented 3 years ago

Above fix works for adafruit's fork of ArduinoCore-samd because they define pgmspace.h. CI failed to compile for OG ArduinoCore-samd... 😦 still exploring the problem.

2bndy5 commented 3 years ago

Looks like the Adafruit/ArduionoCore-samd fork includes a copy of avr/pgmspace.h for backwards compatibility with libraries like RF24. The original ArduinoCore-samd uses a newer unified API called api/ArduinoAPI.h which doesn't include avr/pgmspace.h.

This may be an increasing problem for new Arduino boards/cores going forward (like the upcoming RP2040 - tagging #727 ). The unified ArduinoCore-API repo has a copy of pgmspace.h in a subfolder titled deprecated-avr-comp/avr... I think this move to a unified ArduinoCore-API repo happened around 2016. The AVR core seems to be unaffected since it predates 2016.

@wmarkow The need to switch to a logger type library just got more pressing!

About the missing printf()

Serial.printf() was removed from the ArduinoCore-API repo's Print.h (which gets inherited by Stream.h which is then inherited by HardwareSerial.h). the original call to Serial.printf() (based on work in the Adafruit/ArdionoCore-samd repo) uses vnsprinf() (which I think is included from stdarg.h).