[BUG] HAL_timer_get_count called before HAL_timer_start on certain configurations

[fermino]

Did you test the latest bugfix-2.1.x code?

Yes, and the problem still exists.

Bug Description

Based on the bug #27450 I took on to the task of updating the framework (arduino-espressif32 based on esp-idf) to see if that made any difference. I saw an update to arduino-espressif32@3.5.0 at 4ef53721512776eacc28a0f74c9d158cf8246ad9, but that's still way outdated.

Trying out things, I found that ANY SDK version newer than v3.5.0 (which internally would be a jump from esp-idf v3.5.3 to v4.4) throws the following error in this line:

E (24567) timer_group: timer_get_counter_value(54): HW TIMER NEVER INIT ERROR

Going down that rabbit hole I found that, in certain configurations, some of the timers are being used without being set up first. An good example of this is timer 0 (on an ESP32 with I2S_STEPPER_STREAM enabled). HAL_timer_get_count is called here, even when HAL_timer_start is never called.

Stacktrace for reference:

0x400d3aa3: HAL_timer_get_count(unsigned char) at /home/fermino/develop/marlin-builds/.build/AnetA6/Marlin/src/HAL/ESP32/timers.cpp:162
0x400fc7f5: Stepper::block_phase_isr() at /home/fermino/develop/marlin-builds/.build/AnetA6/Marlin/src/module/stepper.cpp:2389
0x400d2f3f: stepperTask(void*) at /home/fermino/develop/marlin-builds/.build/AnetA6/Marlin/src/HAL/ESP32/i2s.cpp:171

In any case, the newer SDK is not as forgiving as the old one and it throws that error.

One solution would be forcing OLD_ADAPTIVE_MULTISTEPPING to be enabled if I2S_STEPPER_STREAM is enabled, another would be to keep track of which timers have been init'd and init them on demand if necessary. A quick n' dirty workaround would be to just return 0 if the timer was not started.

In any case, I don't feel I know Marlin well enough to figure this one out, so I'm requesting some comments on this one. I don't really want to tag anyone but I feel like @quiret might have some input on this as he is the one that updated the SDK on the first place.

Any input is appreciated :) I will keep doing some testing and if any solution is agreed on I will be happy to make appropiate patches. Kind regards, Fermín Olaiz.

Bug Timeline

It's been there for a while but the stricter SDK makes it noticeable

Version of Marlin Firmware

bugfix-2.1.x

Electronics

BOARD_MKS_TINYBEE

[fermino]

I feel like this would be the appropiate patch, as the timer is used when OLD_ADAPTIVE_MULTISTEPPING is disabled.

--- Marlin/src/module/stepper.cpp     2024-09-28 21:30:05.000000000 -0300
+++ Marlin/src/module/stepper.cpp     2024-10-02 16:59:03.541639004 -0300
@@ -3205,7 +3205,7 @@
     E_AXIS_INIT(7);
   #endif

-  #if DISABLED(I2S_STEPPER_STREAM)
+  #if DISABLED(I2S_STEPPER_STREAM) || DISABLED(OLD_ADAPTIVE_MULTISTEPPING)
     HAL_timer_start(MF_TIMER_STEP, 122); // Init Stepper ISR to 122 Hz for quick starting
     wake_up();
     sei();

[fermino]

I kept digging as the patch above did not fix the case as I thought it would.

Turns out that the timer is set up when the steppers are brought up. But the task that eventually calls for the value from the timer is set up around a hundred lines BEFORE the timer is set up.

If the task is executed before the timer is init'd, it fails.

I will keep trying to think what the best solution would be and will post a patch as soon as I have something. Ideas are welcome :)

[fermino]

Moving the i2s calls up is not possible due to #21019 (which might be fixed in recent arduino-esp32 releases? I haven't found any literature on it yet). The only decent workaround I can think of is to keep track of what timers have been init'd and init them on demand if they're needed before they're set up.