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

fix SERIAL_BUFFER_SIZE for SAMD & nRF5x (Adafruit) #459

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

hathach
Copy link
Contributor

@hathach hathach commented Jul 13, 2020

This PR fixes current master build with SAMD & Adafruit nRF core. PR #448 introduce the usage of SERIAL_RX_BUFFER_SIZE however this symbol is not define on SAMD & nRF core. There is SERIAL_BUFFER_SIZE that should be used

https://github.com/arduino/ArduinoCore-samd/blob/master/cores/arduino/RingBuffer.h#L31

current build errror

SerialFirmata.cpp: In member function 'void SerialFirmata::checkSerial()':
SerialFirmata.cpp:368:56: error: 'SERIAL_RX_BUFFER_SIZE' was not declared in this scope
           if (!((bytesToRead <= 0 && bytesAvailable >= SERIAL_RX_BUFFER_SIZE/2)
                                                        ^~~~~~~~~~~~~~~~~~~~~
/home/hathach/Arduino/libraries/firmata/utility/SerialFirmata.cpp:368:56: note: suggested alternative: 'SERIAL_BUFFER_SIZE'
           if (!((bytesToRead <= 0 && bytesAvailable >= SERIAL_RX_BUFFER_SIZE/2)
                                                        ^~~~~~~~~~~~~~~~~~~~~
                                                        SERIAL_BUFFER_SIZE
Multiple libraries were found for "Firmata.h"

@zfields
Copy link
Contributor

zfields commented Jul 13, 2020

I would request a nested #if, so it made clear the order of precedence for sourcing SERIAL_RX_BUFFER_SIZE.

// The RX and TX hardware FIFOs of the ESP8266 hold 128 bytes that can be 
// extended using interrupt handlers. The Arduino constants are not available
// for the ESP8266 platform.
#if !defined(SERIAL_RX_BUFFER_SIZE)
  #if defined(UART_TX_FIFO_SIZE)
    #define SERIAL_RX_BUFFER_SIZE UART_TX_FIFO_SIZE
  #elif defined(SERIAL_BUFFER_SIZE) // For SAMD and nRF5x core
    #define SERIAL_RX_BUFFER_SIZE SERIAL_BUFFER_SIZE
  #endif
#endif

This appears to be a good candidate for abstraction with a FIRMATA_SERIAL_RX_BUFFER_SIZE variable. Not to mention, when viewed like this, it illustrates we are not guaranteed to have a default value for SERIAL_RX_BUFFER_SIZE, which can be easily supplied with an #else preprocessor command.

@hathach hathach closed this Jul 13, 2020
@hathach hathach reopened this Jul 13, 2020
@hathach
Copy link
Contributor Author

hathach commented Jul 13, 2020

Push to my branch but github doesn't update PR, close & reopen PR as a trick to force update

@hathach hathach changed the title fix SERIAL_BUFFER_SIZE for samd fix SERIAL_BUFFER_SIZE for SAMD & nRF5x (Adafruit) Jul 13, 2020
@zfields
Copy link
Contributor

zfields commented Jul 13, 2020

The code looks good to me!

@soundanalogous has the hardware to run tests before we merge, so I'll let him work his magic.

Thanks for the PR!

@hathach
Copy link
Contributor Author

hathach commented Jul 16, 2020

with the removal of FIRMATA_SERIAL_RX_DELAY for SAMD at commit 6e6b5c8

This PR may not be needed anymore. I am not sure the above commit an interim commit or not. Feel free to merge or close this issue or tell me if you need to make any changes.

@soundanalogous
Copy link
Member

6e6b5c8 fixes the compile issue but doesn't enable support for using a serial buffer with SMAD boards.

I'll look into getting this merged sometime this weekend if I can find some time to test it.

@hathach
Copy link
Contributor Author

hathach commented Jul 19, 2020

thanks, feel free to merge whenever you have time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants