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

ports/rp2/machine_i2s.c: Detect and deinitialize I2S instances on boot. #14345

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

marecl
Copy link

@marecl marecl commented Apr 21, 2024

In reference to #14339

machine_i2s_init0 makes all of these pointers NULL without checking if there was a pointer already. Furthermore, garbage collector doesn't care enough to call I2S.deinit().
This created an issue, where invoking machine.soft_reset() would hard crash an entire board.
Fix is a check if machine_i2s_obj contains an object (assumed to be a valid machine_i2s_obj_t). If memory address is not NULL - object gets deinitialized. No further crashes are observed.

@marecl marecl marked this pull request as draft April 21, 2024 17:37
@marecl marecl force-pushed the master branch 4 times, most recently from 00265f7 to 6705d18 Compare April 21, 2024 20:47
During early init some I2S instances may still be present in buffer.
This issue would cause system to hang when using soft_reset without
deinitializing I2S instance first.
@marecl marecl changed the title Fixed: State of I2S objects saved between soft_reset causing crash ports/rp2/machine_i2s.c: Detect and deinitialize I2S instances on boot. Apr 21, 2024
@marecl marecl marked this pull request as ready for review April 21, 2024 20:54
@dpgeorge
Copy link
Member

Furthermore, garbage collector doesn't care enough to call I2S.deinit().

To fix that, finalisers should be enabled on the I2S object on all ports, following how esp32 does it. That requires two things:

  1. Remove the #if MICROPY_PY_MACHINE_I2S_FINALISER guard from extmod/machine_i2s.c.
  2. Make rp2, stm32, mimxrt ports all use mp_obj_malloc_with_finaliser() instead of mp_obj_malloc() in their mp_machine_i2s_make_new_instance() implementations.

Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:   +32 +0.010% RPI_PICO
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS

@robert-hh
Copy link
Contributor

If you do not want to follow the recommendation of Damien, this deinit should better be placed at the end of main() between the label soft_reset_exit: and the call to gc_sweep_all();. At that point the I2S data structure is still intact and mp_machine_i2s_deinit() can be called safely. After gc_sweep_all() has run, all allocated data elements can not be considered any more as valid, which includes the I2S object and it's buffers. So they must not be used in machine_i2s_init0(). You might have to create a function like machine_i2c_deinit_all() in machine_i2s.c which is called at behind label soft_reset_exit: in main.c.

@marecl
Copy link
Author

marecl commented Apr 25, 2024

If you do not want to follow the recommendation of Damien, this deinit should better be placed at the end of main() between the label soft_reset_exit: and the call to gc_sweep_all();. At that point the I2S data structure is still intact and mp_machine_i2s_deinit() can be called safely. After gc_sweep_all() has run, all allocated data elements can not be considered any more as valid, which includes the I2S object and it's buffers. So they must not be used in machine_i2s_init0(). You might have to create a function like machine_i2c_deinit_all() in machine_i2s.c which is called at behind label soft_reset_exit: in main.c.

I'll work on this, just don't have a lot of free time. I'd rather go with Damien's solution as it seems more appropriate and portable. I don't have other devboards, so most likely fixing it globally (like calling __del__ or __exit__) would free us from meddling with hardware.
Also, sorry about chaotic messaging in this thread but the only time I really have at hand is late at night :P

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

Successfully merging this pull request may close these issues.

None yet

3 participants