Port some AVR Serial_ (SerialUSB) API's over

[sandeepmistry]

Port over the following Serial_ API's over from the AVR core to SAMD:

• readBreak
• baud
• stopbits
• paritytype
• numbits
• dtr
• rts

That were added to AVR in the following commits:

• arduino/Arduino@96f7b96
• arduino/Arduino@b58f239

cc/ @matthijskooijman

[matthijskooijman]

Just had a quick look over this commit, which looks good. Just one atomicity issue to fix.

[sandeepmistry]

@matthijskooijman I've address the atomicity issue by disabling and re-enabling USB interrupts in readBreak (sandeepmistry@de1429b).

Again as mentioned in arduino/Arduino#4171 (comment), any ideas on how to test this condition?

[matthijskooijman]

@sandeepmistry, two more remarks:

• This always enables the interrupt, even if it was disabled before. I don't suppose it will ever be disabled with the current code, but you never know what a sketch will do. In any case, it's good practice to clear the interrupt flag, and then restore it afterwards, instead of forcing it to enabled.
• Are you sure you're disabling the right interrupt? Tracing back, I see the ISR routine is called UDD_Handler, not USB_Handler. I'm just guessing here, I don't really know about the SAM register names.

As for testing this, one trick is to put a delay between the read and write of breakValue, which increases the chances of the race condition triggering. Furthermore, when a few break requests are sent close together, you can miss some of them by not checking frequently enough, but you should always see the last one sent. If the race condition triggers, you can actually miss the most recent one.

To test, you could send a "break on" immediately followed by a "break off" and repeat that over and over again (with a bit of delay in between, so you can be sure that readBreak() was called). If you then let the break condition pull a line low, you should never see it low during the delay. If you do see it low for more than a short pulse, then you'll know you've missed a "break off" and the race condition triggered.

[sandeepmistry]

@matthijskooijman thanks for reviewing and the tips on how to test!

I've addressed your feedback in sandeepmistry@d848264, by taking a similar approach to Serial_::accept and using noInterrupts and interrupts to make the block atomic.

[matthijskooijman]

@sandeepmistry, I actually think this change makes things worse (also, I think the implementation of accept() is also bad, my previous comment also applies there). Now, you disable all interrupts, instead of just the one that is needed (which is commonly done, but in this case not needed). More importantly, interrupts() still unconditionally enables interrupts, causing a call to readBreak() (or accept()) to (globally) enable interrupts if they were disabled. In that sense, things are worse now, since the chance that these functions are called with the global interrupt flag cleared are a lot bigger than with just the USB interrupt flag cleared. In any case, these should just store the state of the interrupt flag and restore it afterwards (ideally just for the USB interrupt, though disabling interrupts globally also prevents issues when e.g. readBreak() is called from within an ISR). I tried to look for an example of this clear-and-restore behaviour in some other code, but couldn't find anything quickly.

[sandeepmistry]

@matthijskooijman good points, I think SPI:usingInterrupt has the approach we need: https://github.com/sandeepmistry/ArduinoCore-samd/blob/master/libraries/SPI/SPI.cpp#L100

It only calls interrupts() if __get_PRIMASK()/interruptsStatus was true. What do you think?

[matthijskooijman]

Yup, that is indeed to solution I was aiming for. I'm not sure if it's worthwile to only disable the USB interrupt, or disable interrupts globally, though. The latter is safer, but only in corner cases that will probably never occur, and causes extra interrupt delay in other cases. But since the delay is minimal, I think I'd favor the global disabling.

On another note, never knew about the interruptsStatus() function, seems its samd (or also sam?) specific and undocumented?

[matthijskooijman]

Oh, just noticed that that function is defined by SPI.cpp itself: https://github.com/sandeepmistry/ArduinoCore-samd/blob/master/libraries/SPI/SPI.cpp#L85-L93

[sandeepmistry]

I'll move interruptsStatus() into a shared place in the SAMD core.

[matthijskooijman]

Note that interruptsStatus() is probably fine to use in internal SAMD code, but I do not think it works well enough as a generally available API due to portability issues. There has been a thread about pretty much this feature a while ago, without any real conclusion. See https://groups.google.com/a/arduino.cc/d/msg/developers/cmu0Qy32zxY/PIo4YWtmAwAJ

[sandeepmistry]

Another changed pushed, I've decided to use __get_PRIMASK directly instead of moving interruptsStatus from the SPI library into the SAMD core.

[matthijskooijman]

That sounds good to me. I dug in a little more (to verify that "PRIMASK" is really the same bit as the one modified by interrupts()/noInterrupts(), and I found that the latter calls __disable_irq() which is defined in ~/.arduino15/packages/arduino/tools/CMSIS/4.0.0-atmel/CMSIS_RTX/SRC/rt_HAL_CM.h as:

__attribute__((always_inline)) static inline U32 __disable_irq(void)
{
  U32 result;

  __asm volatile ("mrs %0, primask" : "=r" (result));
  __asm volatile ("cpsid i");
  return(result & 1);
}

In other words, noInterrupts() already returns the interrupt mask, so it might be even easier to use that (it's not portable, but this code does not need to be). To emphasize the non-portability (and reduce confusion for people reading the code), it might be better to even just use __disable_irq() and __enable_irq() directly?

Also note that the above code limits to just looking at bit 0 and masks out the rest, while the current commit (and SPI code) look at all bits. According to the documentation all bits but 0 are reserved, so this is probably not a problem in practice, but masking would be appropriate.

[sandeepmistry]

It turns out __disable_irq from ~/.arduino15/packages/arduino/tools/CMSIS/4.0.0-atmel/CMSIS_RTX/SRC/rt_HAL_CM.h is not included.

Instead the one from ~/Library/Arduino15/packages/arduino/tools/CMSIS/4.0.0-atmel/CMSIS/Include/core_cmFunc.h is included. Which has a void return type.

/** \brief  Disable IRQ Interrupts

  This function disables IRQ interrupts by setting the I-bit in the CPSR.
  Can only be executed in Privileged modes.
 */
__attribute__( ( always_inline ) ) __STATIC_INLINE void __disable_irq(void)
{
  __ASM volatile ("cpsid i" : : : "memory");
}

Also note that the above code limits to just looking at bit 0 and masks out the rest, while the current commit (and SPI code) look at all bits. According to the documentation all bits but 0 are reserved, so this is probably not a problem in practice, but masking would be appropriate.

I've updated to mask.

[sandeepmistry]

Here's a link to the relevant M0 documentation for my comment above: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0497a/BABEDCBC.html

To emphasize the non-portability (and reduce confusion for people reading the code), it might be better to even just use __disable_irq() and __enable_irq() directly?

Agreed, I've pushed changes for this.

[matthijskooijman]

Instead the one from ~/Library/Arduino15/packages/arduino/tools/CMSIS/4.0.0-atmel/CMSIS/Include/core_cmFunc.h is included. Which has a void return type.

Ah, good point. I just looked at the first one I found using grep :-)

The code looks good now. Before merging, it would probably be good to squash all commits into one? Also, it might be good to let accept() use the same conditional interrupts() call (in a separate commit, of course)? Bit off-topic for this PR, though it would be good to change it anyway.

[sandeepmistry]

The code looks good now. Before merging, it would probably be good to squash all commits into one?

Done, I've squash and rebased.

Also, it might be good to let accept() use the same conditional interrupts() call (in a separate commit, of course)? Bit off-topic for this PR, though it would be good to change it anyway.

I'll open a new PR for this today.

[matthijskooijman]

Looks good to me!