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

commented Jan 15, 2016

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

@avtolstoy avtolstoy force-pushed the avtolstoy:serialfixes branch from 2552ef1 to 30589c6 Jan 15, 2016

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

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Jan 17, 2016

Contributor

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.

int32_t USB_USART_Available_Data_For_Write(void)
{
if (USB_USART_Connected())
{

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Jan 17, 2016

Contributor

Similarly here - because USB_Tx_Buffer_tail is updated by the USB IRQ, it's best to capture that as a local variable first, then use it for comparison and computation to avoid an inconsistent computation.

//
uint32_t USB_Rx_length;
if (USB_Rx_Buffer_head >= USB_Rx_Buffer_tail)
USB_Rx_Buffer_length = USB_RX_BUFFER_SIZE;

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Jan 17, 2016

Contributor

Can you add code comments please to clarify the need for a buffer length in addition to the head/tail pointers?

@avtolstoy

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2016

@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

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Jan 19, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2016

@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
@m-mcgowan m-mcgowan referenced this pull request Jan 21, 2016
avtolstoy added 4 commits Jan 22, 2016
Merge remote-tracking branch 'upstream/develop' into serialfixes
Conflicts:
	hal/inc/hal_dynalib_usart.h
	user/tests/wiring/api/wiring.cpp
	wiring/src/spark_wiring_usartserial.cpp
Merge remote-tracking branch 'upstream/develop' into serialfixes
Conflicts:
	wiring/inc/spark_wiring_usbserial.h
@avtolstoy

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2016

Also fixes #669 and #846

@technobly

This comment has been minimized.

Copy link
Member

commented Feb 9, 2016

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 avtolstoy force-pushed the avtolstoy:serialfixes branch from 1d57384 to 3f495bf Feb 10, 2016

@avtolstoy

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2016

@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
Merge pull request #812 from avtolstoy/serialfixes
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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@m-mcgowan m-mcgowan referenced this pull request Mar 10, 2016
17 of 17 tasks complete
@technobly

This comment has been minimized.

Copy link
Member

commented Mar 25, 2016

This PR also solves Issue #923

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.