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

Support for USART parity. #757

Merged

Conversation

@TrevorN
Copy link
Contributor

commented Dec 7, 2015

Based off of the work of @jmekler in pull request #413, but with support for all platforms and rebased off the head of develop.

To use even/odd parity, pass the corresponding configure flag when invoking SerialN.begin

One Stop Bit:

SERIAL_8N1 -> No Parity
SERIAL_8O1 -> Odd Parity
SERIAL_8E1 -> Even Parity

Two Stop Bits:

SERIAL_8N2 -> No Parity
SERIAL_8O2 -> Odd Parity
SERIAL_8E2 -> Even Parity

  • Check for signed CLA
  • Review code
  • Test on device
  • Add documentation
  • Add to CHANGELOG.md
@bwheeler96

This comment has been minimized.

Copy link

commented Dec 9, 2015

Other than a few nits looks really solid. I could really use this feature support 👍

@tomtruitt

This comment has been minimized.

Copy link

commented Dec 28, 2015

Guys i really need 9n1... Is this possible?

@TrevorN

This comment has been minimized.

Copy link
Contributor Author

commented Jan 2, 2016

@tomtruitt Unfortunately it doesn't look like there is hardware support for 9N1, but it should be possible to create that functionality through software. That is out of the scope of this pull request, however.

@tomtruitt

This comment has been minimized.

Copy link

commented Jan 3, 2016

@TrevorN

Hi thanks for your reply!

This is from some one at Particle support to me: "Yes. There is certainly hardware support and the framework for firmware support."

Unfortunately I don't have enough understanding to discern for myself.

Also I was provided with this link: https://github.com/spark/firmware/blob/aefb3342ed50314e502fc792f673af7a74f536f9/platform/MCU/STM32F1xx/STM32_StdPeriph_Driver/inc/stm32f10x_usart.h#L129

If this question is outside the scope of this pull request I apologize. Could you point me in a direction to proceed to learn more about if this is possible? Thank you

@TrevorN

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2016

@tomtruitt Unfortunately the 9 in the 'USART_WordLength_9b' parameter includes the parity bit, so it is not possible in hardware to have a 9 bit message with a 10th parity bit. If you are looking to transmit an 8 bit message with an additional parity bit, this pull request will satisfy that requirement.

In the datasheet for this processor:
screen shot 2016-01-03 at 12 34 08 pm

@TrevorN

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2016

@m-mcgowan This pull request has been tested on the Photon and is functional, are there any changes that need to be made before this can be merged into develop? Serial parity is an essential feature, and my team–among others–requires it for our product.

Thanks for your consideration!

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2016

I would change the config param from uint8_t to uint32_t to provide more room for additional flags.

On the stm32f2xx we use dynamic linking. Please see the contributions doc which outlines the changes permitted. Speicifcally, it's not possible to add a parameter to an existing function. Rather, a new function should be added to the end of the dynamic linking table. (E.g. HAL_USART_BeginConfig). As well as taking a uint32_t configuration parameter, it's a good idea to add a void* parameter for future expansion.

@m-mcgowan m-mcgowan added this to the 0.4.9 milestone Jan 3, 2016

@@ -39,6 +39,8 @@ typedef enum USART_Num_Def {
#define GPIO_Remap_None 0

/* Private macro -------------------------------------------------------------*/
// IS_USART_CONFIG_VALID(config) - returns true for 8 data bit, any flow control, any parity, any stop byte configurations
#define IS_USART_CONFIG_VALID(CONFIG) ( (((CONFIG & 0b00001100)>>2) != 0b11) && (((CONFIG & 0b00110000)>>4)==0b11) )

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Jan 14, 2016

Contributor

It looks like this would fail when CONFIG==SERIAL_8N1 (0). Can you confirm please that you've tested this case?

This comment has been minimized.

Copy link
@sergeuz

sergeuz Jan 21, 2016

Member

I second the question. (CONFIG & 0b00110000)>>4)==0b11 part doesn't seem to check anything that is used in HAL_USART_BeginConfig() later, and fails for SERIAL_8N1.

@m-mcgowan m-mcgowan removed this from the 0.4.9 milestone Jan 19, 2016

@flaz83

This comment has been minimized.

Copy link

commented Jan 27, 2016

Hi,
I should use serial 9N1, 9 data bits, no parity, 1 stop bit.
Is it possible?
probably something like that:

USART_InitStructure.USART_BaudRate = 9600;
USART_InitStructure.USART_WordLength = USART_WordLength_9b;
USART_InitStructure.USART_StopBits = USART_StopBits_1;
USART_InitStructure.USART_Parity = USART_Parity_No;
USART_InitStructure.USART_HardwareFlowControl = USART_HardwareFlowControl_None;
USART_InitStructure.USART_Mode = USART_Mode_Rx | USART_Mode_Tx;
USART_Init(USART3, &USART_InitStructure);

Thank you so much,
Flavio

@tomtruitt

This comment has been minimized.

Copy link

commented Jan 27, 2016

@TrevorN shows how much I know I thought the 9th bit was the stop bit.

@flaz83 this question was apparently answered for me earlier.

@tomtruitt

This comment has been minimized.

Copy link

commented Jan 27, 2016

@TrevorN wait I'm confused by your response. 9n1 is no parity bit so that's not an issue. Is the stop bit an issue?

@flaz83

This comment has been minimized.

Copy link

commented Jan 27, 2016

@tomtruitt Sorry, I don't understand. I don't need a parity bit, but 9bit + stop bit.

@tomtruitt

This comment has been minimized.

Copy link

commented Jan 27, 2016

@flaz83 agreed yes I was confused by the answer as that's what I need as well

@flaz83

This comment has been minimized.

Copy link

commented Jan 27, 2016

@tomtruitt I'm looking for 9bits + 9nth stop bit, no parity

@TrevorN

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2016

@tomtruitt 8 bits plus a 9th stop bit is 8N1. The stop bit is not counted in the 8 bits.

@flaz83 unfortunately the hardware only has an 8 bit output register, so you can't have any packet with more than 8 data bits.

@tomtruitt

This comment has been minimized.

Copy link

commented Jan 27, 2016

On item number 1 of the image you uploaded it seems 9n1 would be capable. I'm sorry I'm not understanding the discrepancy

@tomtruitt

This comment has been minimized.

Copy link

commented Jan 27, 2016

@flaz83 yes i believe we are both looking for a device that is 9n1 capable with connectivity.

@TrevorN

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2016

9N1 would be 9 data bits + no parity bits + 1 stop bit. This configuration isn't is supported. (what @flaz83 is asking for)

8N1 would be 8 data bits + no parity bits + 1 stop bit. This configuration is supported. (what @tomtruitt is asking for)

Edit: Looks like I was wrong. You can do 9 data bits, but only without parity.

@TrevorN

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2016

@tomtruitt Sorry, looks like I was reading the manual incorrectly.
screen shot 2016-01-27 at 3 16 41 pm
You can have 9 data bits, but only if there is no parity.

@tomtruitt

This comment has been minimized.

Copy link

commented Jan 27, 2016

Great this exactly what we need. I'm not very experienced with git. Should I ask my developer to create a new a pull request then? What is the next step?

@TrevorN

This comment has been minimized.

Copy link
Contributor Author

commented Jan 27, 2016

@tomtruitt I modified the firmware to support 9N1. I currently don't have access to a photon, do you mind testing these changes with your hardware?

@tomtruitt

This comment has been minimized.

Copy link

commented Jan 28, 2016

Oh excellent! I actually don't have the hardware as I was waiting to learn of 9n1 support before purchasing.

@flaz83

This comment has been minimized.

Copy link

commented Jan 29, 2016

Thank you so much!!!! I'm porting my software from arduino to photon and I try.
Could you please tell me how to use your code?

Serial2.begin(9600, SERIAL_9N1);
Serial2.write(0x112);
is it correct?

@tomtruitt

This comment has been minimized.

Copy link

commented Jan 29, 2016

@flaz83 I can't help but wonder if we are working on similar projects... My email is Tomtruitt@libertyvending.org if you care to chat.

@@ -29,7 +29,7 @@
void HAL_USART_Init(HAL_USART_Serial serial, Ring_Buffer *rx_buffer, Ring_Buffer *tx_buffer)
{
}
void HAL_USART_Begin(HAL_USART_Serial serial, uint32_t baud)
void HAL_USART_Begin(HAL_USART_Serial serial, uint32_t baud, uint8_t config)

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Mar 10, 2016

Contributor

I think this should be HAL_USART_BeginConfig(HAL_USART_Serial serial, uint32_t baud, uint8_t config, void*)

@@ -31,6 +31,16 @@
#include "usart_hal.h"
#include "spark_wiring_platform.h"

//Available Serial Configuration for C

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Mar 10, 2016

Contributor

Could you please move these constants down into the HAL layer, into usart_hal.h since they are part of the HAL interface. At present, the new code in the HAL contains implicit magic constants for the usart config - having these available to the HAL will mean we can use more meaningful symbols instead.

This comment has been minimized.

Copy link
@clay-to-n

clay-to-n Mar 10, 2016

Contributor

I can move it down, but these constants don't quite match the checks happening in usart_hal.cpp. I could define those too though, such as:
#define SERIAL_STOP_BITS 0b00000011
#define SERIAL_PARITY_BITS 0b00001100
etc.

Is this what you mean?

@@ -51,12 +61,13 @@ class USARTSerial : public Stream
virtual int peek(void);
virtual int read(void);
virtual void flush(void);
virtual size_t write(uint16_t);

This comment has been minimized.

Copy link
@m-mcgowan

m-mcgowan Mar 10, 2016

Contributor

The write 16-bit overload doesn't need to be virtual since it's not part of the Stream interface. (If we ever made a WideCharStream then it would be virtual, but let's cross that bridge when we get to it! ;-))

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

I've made some inline comments after making a pass through the code. Overall, this looks good, thanks for the contribution @TrevorN!

To be included in the next release, we need a few other supporting changes (as detailed in the CONTRIBUTING.md doc in the root of this repo):

  • a documentation PR to the spark/docs repo with documentation for the new feature
  • compile-time API assertions in the user/tests/wiring/api
  • additional test cases in the user/tests/wiring/serial_loopback to test the new usart word size, stop bit and and parity settings.

I hope that's clear - I'm happy to provide guidance where needed.

@clay-to-n

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

@m-mcgowan Could I get some more guidance on your second point? I've found a test(api_wiring_usartserial) function in user/tests/wiring/api/wiring.cpp.

Is this where I should add additional API_COMPILE statements relating to USART configurations?

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

Yes, that's the right place to put some compile-time tests for the new usart config APIs.

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2016

We want to make a pre-release this week - if you can have the changes ready then we'll include this!

@davismwfl

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2016

@m-mcgowan I have written the compile time assertions and written tests in the serial_loopback test. When I go to test on device for the serial loopback I get disconnected from the device and it reboots as soon as the call to Serial1.begin(...) executes. I do have a jumper between TX and RX. And this includes the 1 pre-existing test for Serial1 as well, not just my new ones.

I am sure I am missing something obvious. If I run the "Serial" test in the serial_loopback it works fine. I'd like to get this done so I can push the tests for review, so any pointers will be greatly appreciated.

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2016

I tried the serial_loopback on the develop branch and it's working for me. Be sure you've flashed system firmware to the device with the changes in this PR.

Please push your latest changes, rebased against develop and I'll help investigate.

m-mcgowan and others added 2 commits Mar 16, 2016
Merge branch 'feature/hal-usart-parity-options' of https://github.com…
…/TrevorN/firmware into TrevorN-feature/hal-usart-parity-options

# Conflicts:
#	hal/inc/hal_dynalib_usart.h
@clay-to-n

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2016

Created a documentation pull request for this feature here: particle-iot/docs#317

davismwfl and others added 2 commits Mar 16, 2016
Merge pull request #2 from davismwfl/feature/hal-usart-parity-options
Added Assert and Serial Loopback tests for Serial Parity features.
@clay-to-n

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2016

Okay, this should have tests and API assertions now thanks to @davismwfl.

@davismwfl

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2016

@m-mcgowan and others. My original post and issue with the tests not running properly on the device were resolved by making sure I was building the firmware from the firmware/module directory and not firmware/main.

To add to some of my confusion the serial_loopback test in the branch prior to merging with develop would flash to the device and partially run but fail when using Serial1. After the merge with DEVELOP it would fail in SOS mode which was more telling. After re-reading the documentation once I built from firmware/modules for the firmware and firmware/main for tests everything ran properly and was testable.

@m-mcgowan m-mcgowan added this to the 0.5.x milestone Mar 17, 2016

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2016

Nice work everyone!

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2016

@avtolstoy Could you please review, run the tests, and merge this ASAP! Thanks! Nevermind, I did it. ;-)

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2016

Tested on Core, Photon, Electron, all green.

m-mcgowan added a commit that referenced this pull request Mar 17, 2016

@m-mcgowan m-mcgowan merged commit 0384e46 into particle-iot:develop Mar 17, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@clay-to-n

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2016

Woohoo!

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