cli() in tone/waveform, needed?

[ghost]

Hi!

I found a cli() (disable interrupts right?) call here:

Arduino/cores/esp8266/core_esp8266_waveform.c

Line 214 in 91bb97d

cli();

Is that needed at all? Why is it there? :)
It seems the only use in that source file, nowhere to re-enable interrupts, isn't that a problem?

Mostly trying to understand the code here, hope it's ok to have an issue for that :)

Thanks!

[devyte]

The issue template has been completely ignored. However, given that the referenced code is brand new, and that the description looks valid to me, I'm keeping this open.
@earlephilhower is that cli() call left over, or is there a matching sei() call missing?

[ghost]

Oh, you never see the template if you create an issue by clicking a line of code in the repository, sorry :)

[earlephilhower]

Good eyes, @cranphin . It was leftover, or so I thought.

I re-read the code again this morning and I see that while I thought I had guaranteed safe waveform start/stop without disabling IRQs, there is a 2-4 instruction window where nextTimeHighCycles could be trashed by the IRQ if it's called and updates the state for this waveform (due to the bitfield packing used to save memory).

I'll throw in a patch with proper IRQ disable/enable around the startWaveform region of danger late this week or early next.