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

Serial.availableForWrite() and Serial.blockOnOverrun() implementation, USB Serial fixes for STM32F2 #812

Merged
merged 10 commits into from
Mar 10, 2016

Conversation

avtolstoy
Copy link
Member

Added Serial.availableForWrite() method for hardware USARTs and USB Serial.
Added Serial.blockOnOverrun(bool) method to set blocking/non-blocking behavior on TX buffer overrun (default is blocking).

This PR also includes a number of fixes for USB Serial driver on STM32F2.

Closes #798

{
return (USB_Rx_length - USB_Rx_ptr);
int32_t available = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm concerned that USB_Rx_Buffer_head / USB_Rx_Buffer_tail will be changed by the USB IRQ after being tested.

As a minimum, capture the tail and head into a local variables, and use that so we are certain they are consistent.

A context switch after comparing head >= tail could see head roll over to the start of the buffer, which would then result in the incorrect size being returned.

@avtolstoy
Copy link
Member Author

@m-mcgowan I've update PR with some minor adjustments.

Yeah, I was also concerned about possible computation inconsistencies. As I said earlier, the only places that could really use a critical section are USB_USART_Available_Data and USB_USART_Receive_Data as they depend on both USB_Rx_Buffer_length, USB_Rx_Buffer_tail and USB_Rx_Buffer_head which might get modified in interrupt handler in the middle of computation.

To have a clearer picture:
TX:

  • USB_Tx_Buffer_head is only modified "atomically" from user code in USB_USART_Send_Data
  • USB_Tx_Buffer_tail is only modified "atomically" in interrupt handler in usbd_cdc_Schedule_In or usbd_cdc_DataIn

RX:

  • USB_Rx_Buffer_tail may be modified from user code in USB_USART_Receive_Data or from interrupt handler in usbd_cdc_Start_Rx when the circular buffer needs to be wrapped to accomodate CDC_DATA_OUT_PACKET_SIZE.
  • USB_Rx_Buffer_length (which keeps track of available contiguous buffer space in USB_Rx_Buffer) is the most problematic as it gets modified in interrupt handler in usbd_cdc_Start_Rx and might be modified along with USB_Rx_Buffer_tail and USB_Rx_Buffer_head.
  • USB_Rx_Buffer_head is only modified "atomically" from interrupt handler in usbd_cdc_DataOut or usbd_cdc_Start_Rx when when the circular buffer needs to be wrapped to accomodate CDC_DATA_OUT_PACKET_SIZE.

So, just to be on a safe side, I've added critical sections in both USB_USART_Available_Data and USB_USART_Receive_Data.

I've also increased USB RX buffer size to 256, as 128 can only hold 2x CDC_DATA_OUT_PACKET_SIZE packets, and if the host starts to send data in blocks rather than by byte, RX buffer might get overflown easily at higher baudrates. It still works well with 128 as we now notify the host with NACK, but if there is enough RAM I would rather suggest to have it at 256.

@m-mcgowan
Copy link
Contributor

Now that the buffering has been reworked, I'm thinking it would be possible to implement flush() correctly? i.e. wait until any data written has been flushed to the device.

USB, flush, as with write, would be a no-op if not connected.

@m-mcgowan
Copy link
Contributor

The HAL_disable_irq/HAL_enable_irq pairs should preserve the previous IRQ state, like this:

int state = HAL_disable_irq();
// ... atomic stuff
HAL_enable_irq(state);

@m-mcgowan m-mcgowan added this to the 0.4.10 milestone Jan 20, 2016
@avtolstoy
Copy link
Member Author

@m-mcgowan I've updated the PR. Added:

  • flush() implementation for USB Serial
  • Serial port state (open/closed) detection
  • Fixed HAL_disable_irq()/HAL_enable_irq()

m-mcgowan added a commit that referenced this pull request Jan 21, 2016
@avtolstoy
Copy link
Member Author

Also fixes #669 and #846

@technobly
Copy link
Member

I'm testing this branch by making a copy of develop (35a7851), cherry-pick'ing PR #845 (4c388c8) and then merging this PR #812 into that. The following code should sleep for 60 seconds (D7 should light for 62 seconds), but what you see if D7 lit for 2 seconds. Basically the same problem as issue #846, however this time I'm using D1 (D0 has same behavior though). Is there another PR I need to merge in to achieve maximum sleepiness? 😴 💤

#include "application.h"

SYSTEM_MODE(SEMI_AUTOMATIC);

// ALL_LEVEL, TRACE_LEVEL, DEBUG_LEVEL, INFO_LEVEL, WARN_LEVEL, ERROR_LEVEL, PANIC_LEVEL, NO_LOG_LEVEL
Serial1DebugOutput debugOutput(9600, ALL_LEVEL);

int count = 0;

void setup() {
    pinMode(D1, INPUT_PULLDOWN);
    pinMode(D7, OUTPUT);
    Particle.connect();
    DEBUG_D("\r\n[ Particle.connect ]\r\n\r\n");
    waitUntil(Particle.connected);
    DEBUG_D("\r\n[ Particle.connected ]\r\n\r\n");
}

void loop() {
    DEBUG_D("\r\n[ Going to sleep ]\r\n\r\n");
    digitalWrite(D7, HIGH);
    delay(1000);
    System.sleep(D1, RISING, 60);
    delay(1000);
    digitalWrite(D7, LOW);
    DEBUG_D("\r\n[ Waking up and publishing twice! ]\r\n\r\n");
    Particle.publish("b", String(++count));
    delay(3000);
    Particle.publish("b", String(++count));
    delay(3000);
}

@avtolstoy
Copy link
Member Author

@technobly I believe 3f495bf should fix this issue. Apparently USARTs busy transferring data might prevent CPU from entering WFI.

m-mcgowan added a commit that referenced this pull request Mar 10, 2016
Serial.availableForWrite() and Serial.blockOnOverrun() implementation, USB Serial fixes for STM32F2
@m-mcgowan m-mcgowan merged commit 8a43301 into particle-iot:develop Mar 10, 2016
@m-mcgowan m-mcgowan mentioned this pull request Mar 10, 2016
17 tasks
@technobly
Copy link
Member

This PR also solves Issue #923

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.

Serial.availableForWrite function
3 participants