startWrite does not toggle CE for long enough on STM32duino boards

[arthurfprecht]

What radio module do you use?

nRF24l01 PA+LNA (GT24 module)

What driver board(s) do you use?

STM32F411CE (a.k.a Black Pill development board)

If using Linux, what OS are you using?

No response

If using Linux, what RF24 driver did you select?

None

Describe your problem

Environment

I am using Arduino-compatible IDE (Sloeber 4.4.3), STM32Duino for the board package, RF24 library latest version. nRF24l01 PA + LNA boards are on both ends. Both transmitter and receiver are STM32s, TX is STM32F411 and receiver is STM32F103.

Overview

The method startWrite does not toggle CE for long enough on STM32duino boards, causing it to malfunction (transmission mode is never reached, toggle is shorter than 10 us).

Details

The method startWrite can call delayMicroseconds(10) to ensure CE is toggled for long enough time based on the CPU frequency. The CPU frequency (F_CPU) is checked during compile time, but for STM32Duino boards, F_CPU is changed during runtime (upon board initialization). This causes no delayMicroseconds() to be called and a very short CE toggle to happen.

Proposed solution

Substituting the current delay part of startWrite method with the following code fixed the issue (making the F_CPU check happen during runtime):

#if !defined(F_CPU)
   delayMicroseconds(10);
#endif
   if (F_CPU > 20000000)
       delayMicroseconds(10);

What is the RX code?

#include <RF24.h>
// ...

What is the TX code?

// The code for RX is same for TX.
// See above code for RX

[2bndy5]

but for STM32Duino boards, F_CPU is changed during runtime (upon board initialization)

This is news to me. Do have a link to the line(s) of code where this happens?
I see https://github.com/stm32duino/Arduino_Core_STM32/blob/f2dc38281b235c4805427deedcd77d3ebcd334c6/libraries/SrcWrapper/inc/stm32_def.h#L69-L71
and then it is set via various boards' HAL: https://github.com/search?q=repo%3Astm32duino%2FArduino_Core_STM32+%22SystemCoreClock+%3D%22&type=code

making the F_CPU check happen during runtime

This proposed runtime overhead should only be applied to the stm32duino platform. We don't want regressive runtime overhead for other platforms.

#if !defined(F_CPU) || F_CPU > 20000000
    delayMicroseconds(10);
#endif
#ifdef ARDUINO_ARCH_STM32
    if (F_CPU > 20000000)
        delayMicroseconds(10);
#endif

STM32Cube support

There is a rather stale WIP branch that aims to support the STM32Cube IDE (see also #872). Additional precautions might be needed to ensure any STM32 (not only on ARDUINO platform) is affected by the proposal. This would be a long #ifdef line where the compiler checks for macros like STM32F4xx or whatever HAL is actually used. In this case we might also just use #ifdef RF24_STM32 generically (defined by library portability file utility/stm32cube/RF24_arch_config.h).

If the STM32 HALs (used in STM32Cube) don't define F_CPU or they do but only adjust it at compile time, then this consideration is nullified.

Update: Given the findings above (see links), the F_CPU is indeed specific to the Arduino framework. No additional compensations are needed to support STM32Cube IDE.

[2bndy5]

I also found compiler errors (for Generic STM32F4 series) in the scanner.ino example about using max() and min(). Here's the patch to fix that:

--- a/examples/scanner/scanner.ino
+++ b/examples/scanner/scanner.ino
@@ -141,7 +141,7 @@ void loop(void) {
   if (Serial.available()) {
     int8_t c = Serial.parseInt();
     if (c >= 0) {
-      c = min(125, max(0, c));  // clamp channel to supported range        
+      c = min((int8_t)125, c);  // clamp channel to supported range        
       constCarrierMode = 1;
       radio.stopListening();
       delay(2);
@@ -199,7 +199,7 @@ void loop(void) {
     // Print out channel measurements, clamped to a single hex digit       
     for (int i = 0; i < num_channels; ++i) {
       if (values[i])
-        Serial.print(min(0xf, values[i]), HEX);
+        Serial.print(min((uint8_t)0xf, values[i]), HEX);
       else
         Serial.print(F("-"));
     }

[2bndy5]

There are other instances where RF24 lib expects F_CPU to be compile-time exclusive.

RF24/RF24.cpp

Lines 1999 to 2019 in 8aa7103

	#if !defined(F_CPU) || F_CPU > 20000000
	txDelay = 280;
	#else //16Mhz Arduino
	txDelay = 85;
	#endif
	if (speed == RF24_250KBPS) {
	#if !defined(F_CPU) || F_CPU > 20000000
	txDelay = 505;
	#else //16Mhz Arduino
	txDelay = 155;
	#endif
	// Must set the RF_DR_LOW to 1; RF_DR_HIGH (used to be RF_DR) is already 0
	// Making it '10'.
	return static_cast<uint8_t>(_BV(RF_DR_LOW));
	}
	else if (speed == RF24_2MBPS) {
	#if !defined(F_CPU) || F_CPU > 20000000
	txDelay = 240;
	#else // 16Mhz Arduino
	txDelay = 65;
	#endif

but users can adjust RF24::txDelay (after RF24::begin() and every time the data rate is changed) as needed.

The numerous uses of F_CPU in RF24::csn() don't apply to STM32 because that core's SPI library defines SPI_HAS_TRANSACTION.

[2bndy5]

@arthurfprecht I pushed a proposal to a new branch named stm32-fixes (see diff in #1011). If you could test that out, it would be greatly appreciated.

[arthurfprecht]

Hi Brendan, first thank you very much for the quick reply. Good that you found in detail how the clock configuration work. It is somewhat odd, because some other STM32F boards (like the F103C) have pre-set CPU clocks via #define. Reallly good findings related to the other parts of the code impacted by this assumption. I will try to test the library still today and I will report back.

Edit: Tested the code from branch stm32-fixes. The startWrite method now works flawlessly, also the scanner works as intended. Thank you very much for fixing the issue 😄