Skip to content

nrf: neopixel_write: busy poll for one pixel writes #2319

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

Closed
wants to merge 1 commit into from

Conversation

jepler
Copy link

@jepler jepler commented Nov 25, 2019

It's probably not the whole story, however, this fixes a crash observed when bulk copying data to an nRF board using dd.

Basically, the call stack looked like this when resetting into safe mode:

    #0 reset_into_safe_mode reason=reason@entry=GC_ALLOC_OUTSIDE_VM
    #1 gc_alloc
..  #4 external_flash_write_block
.. #11 usb_background
   #12 run_background_tasks
   #13 common_hal_neopixel_write
.. #18 start_mp

i.e., during early startup, it is not okay yet to call allocation functions like m_malloc_maybe that use the garbage collected heap. However, nRF's neopixel_write (which already includes special handling to avoid heap allocations for the status pixel!) can enter background tasks, which do nearly arbitrary things including heap allocations.

We re-use the same test that switches from heap allocation to stack allocation for the pattern buffer (just with the opposite sense)

Testing performed: on an nRF52840 feather, repeatedly dd if=/dev/zero bs=512 count=100 of=/media/jepler/CIRCUITPY/zero conv=fdatasync status=progress. Before this change, at least 50% of trials would cause the board to disconnect from USB and enter safe mode; after this change, 10 iterations completed without trouble.

It's probably not the whole story, however, this fixes a crash observed
when bulk copying data to an nRF board using `dd`.

Basically, the call stack looked like this when resetting into safe mode:
    #0 reset_into_safe_mode reason=reason@entry=GC_ALLOC_OUTSIDE_VM
    #1 gc_alloc
..  #4 external_flash_write_block
.. adafruit#11 usb_background
   adafruit#12 run_background_tasks
   adafruit#13 common_hal_neopixel_write
.. adafruit#18 start_mp

i.e., during early startup, it is not okay yet to call allocation functions
like m_malloc_maybe that use the garbage collected heap.  However,
nRF's neopixel_write (which already includes special handling to avoid
heap allocations for the status pixel!) can enter background tasks, which
do nearly arbitrary things including heap allocations.

We re-use the same test that switches from heap allocation to stack
allocation for the pattern buffer.
Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the long run, the background tasks should probably be careful to note whether it's safe to use the heap (is the VM running?) and act accordingly. But I think this is a good solution for now.

@dhalbert dhalbert requested a review from tannewt November 25, 2019 14:28
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this fix is at the right level. Background tasks should not assume the heap is available. The external flash code should know this and adapt (here). What is the rest of the traceback? I think the fix should be lower where the actual allocation is happening.

@jepler
Copy link
Author

jepler commented Nov 29, 2019

Since this fix is not "at the right level", there's no point in keeping this one open. I'll go the issue route instead, so we can hash out how better to fix it there.

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

Successfully merging this pull request may close these issues.

3 participants