Port Serial_::readBreak() API to SAM core

[sandeepmistry]

Port of the AVR Serial_::readBreak() API (b58f239) to the SAM core.

Resolves #4094, as SAM core was not correctly handling receiving a CDC_SEND_BREAK and sending a stall, causing OS X to report the serial port as "busy". (OS X sends the break when closing the serial port).

cc/ @matthijskooijman

[sandeepmistry]

@matthijskooijman I've pushed a change (sandeepmistry@2c1462d) to disable and re-enable USB OTG interrupts to preventing overwriting the breakValue with -1.

Any suggestions on how to test this condition, it's difficult to distinguish between missing a break from not reading frequently enough and overwriting break value.

[matthijskooijman]

arduino/ArduinoCore-samd#65 (comment) also applies here.

[sandeepmistry]

@matthijskooijman I've addressed your feedback in sandeepmistry@6ffc96b, and have taken a similar approach to Serial_::accept, by using the ARM exclusive access instructions (as suggested by @cmaglie).

[sandeepmistry]

@matthijskooijman this is inline with the SAMD changes now, would you mind reviewing again? Then I can squash the commits and rebase.

[matthijskooijman]

Looks ok to me.

I had not seen your "exclusive access" approach here before, though (somehow missed that commit). Looking more closely now, that does also seem like it could work. I don't think your previous attempt is correct (since the exclusive store could fail, requiring the entire load-store sequence to be retried), though.

At first glance, this exclusive access stuff does not protect against modification by an interrupt handler, only other processors or cores in a multicore setup, but it seems that an interrupt between exclusive load and store will also cause store to fail, so that would also work. An advantage of using the exclusive access primitives would be that it also helps in multicore setups (disabling interrupts only helps with a single core), though that is not really interesting for the current hardware. Another advantage is that by retrying when an interrupt is received, you gain safety at the cost of (potential) slowdown in the main loop, without delaying any interrupt at all. (on a sidenote, I thought this would also be possible to implement in software, e.g. by manually setting an "an interrupt has occurred"-bit in interrupt handlers, but doing this requires a way to atomically store a value, but only if the "an interrupt has occured"-bit is not set (which on e.g. AVR requires disabling interrupts again, I believe).

I suspect that the M0 does not support these access primitives (and probably not multicore or multiprocessor setups either), so just disabling interrupts is the proper solution there? If so, it might be better to do the same here, in the interest of consistency.

[sandeepmistry]

I suspect that the M0 does not support these access primitives (and probably not multicore or multiprocessor setups either), so just disabling interrupts is the proper solution there? If so, it might be better to do the same here, in the interest of consistency.

Yes, this is correct the M0 doesn't support those instructions.

I don't think your previous attempt is correct (since the exclusive store could fail, requiring the entire load-store sequence to be retried), though

Yes, it was not working if any interrupt (not just a USB one) was triggered between the load and store. I was testing using delay as you had suggested earlier.

There is small loss when retrying the load-store sequence, you would miss the previous break value, if a break occurs while reading. I'm still open to making this change though, but you do bring up a good point of doing the same as the SAMD core here.

[matthijskooijman]

There is small loss when retrying the load-store sequence, you would miss the previous break value, if a break occurs while reading.
A, good point. On the other hand, that will probably (almost) never happen, while delaying an interrupt by a few clockcycles will happen more often (or less seldomly, perhaps).

I'm not sure what is the best route to take here, a point can be made for both. I'm inclined to keep the __disable_irq() approach, because it is simpler, and consistent (in code) with SAM and (in concept) with AVR as well.

[sandeepmistry]

Ok, let's stick with the __disable_irq() approach then.

I've squashed my later commits and rebased.

[matthijskooijman]

Looks good!

[sandeepmistry]

@matthijskooijman indentation update is in dcfcbef

Again, thanks for reviewing!