STM32s Serial: Correct handling of parity bits #4216

fmanno · 2017-04-22T14:43:53Z

Description

Reworked the serial_format() function for STM32F0x devices to take the format in the form:
data_bits - parity - stop_bits
(E.g. 8 - N - 1) where data_bits exclude the parity bit.
Added a case for 7 bits data as at least STM32F0x1/STM32F0x2/STM32F0x8 support 7 bits data.
Consolidated serial_format() and uart_init() functions into a general TARGET_STM serial_api.c file since the functions are common to all STM targets.

Status

DONE

Migrations

YES
After this is merged, behaviour for STM32s will be consistent with the API behaviour for other manufacturers. Users won't be able to specify 9bits data + (parity != none). Users will have to review their applications to ensure the way they specify their format takes into account that the parity bit is no longer included in the data_bits count.

Fixes #4189

LMESTM

LGTM

0xc0170 · 2017-04-24T10:05:33Z

IN DEVELOPMENT

What is missing? How did you test this changeset?

This is for STM32F0, what about other families? Do they have the same problem?

0xc0170 · 2017-04-24T10:05:44Z

/morph test

bcostm · 2017-04-24T11:10:27Z

Thanks for this PR. OK for me.

fmanno · 2017-04-24T19:00:34Z

@0xc0170 Honestly, I don't own a STM32F0 so I couldn't test it on real hardware. I could do a unit test that runs on the host if you point me in the direction of some documentation. I tried mbed test in a "mbed-os-example-blinky" project but that didn't work.

Regarding the other families, that's why I marked it as "IN DEVELOPMENT" I admit I didn't look at all the STM32 targets but STM32F0, STM32F3 and STM32F4, at least, have this problem. I think with a little bit more effort a great deal of the code in serial_api.c of each family could be consolidated into a general STM32::serial_api.c
There are a few functions that on a first impression look very specific, but by doing a diff between say STM32F3::serial_api.c and STM32F4::serial_api.c a large number of lines are exactly the same and some differences are just macro names or switch vs. if statements, etc.

The idea of this PR was to at least get a feeling if this was the desired behaviour.
I could try, as a first step and an exercise of feasibility, to make a general STM32::serial_api.c with only the serial_format() function.

mbed-bot · 2017-04-24T20:46:44Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 77

All builds and test passed!

0xc0170 · 2017-04-25T08:55:14Z

The idea of this PR was to at least get a feeling if this was the desired behaviour.
I could try, as a first step and an exercise of feasibility, to make a general STM32::serial_api.c with only the serial_format() function.

That would be ideal if possible.

This shall be good to go or shall we wait for fixing all STM32?

fmanno · 2017-04-25T09:30:17Z

I can try and push something later tonight but can't make any hard promises. I'd like to look at some more datasheets, especially for the other families to check it's also the case for the STM32L variants as well. Regarding testing I only have a STM32F4 nucleo board where I can test it. I'm guessing your CI tests will test some other HW, is that correct?

fmanno · 2017-04-25T12:36:15Z

Looks to be the same for the STM32L family.

Parity generation in transmission
If the PCE bit is set in USART_CR1, then the MSB bit of the data written in the data register
is transmitted but is changed by the parity bit (even number of “1s” if even parity is selected
(PS=0) or an odd number of “1s” if odd parity is selected (PS=1)).

fmanno · 2017-04-25T23:43:49Z

I've updated the PR moving serial_format() and init_uart() into a common source file for all STM targets. I've built all STM targets successfully (some failed but failed the same for the master branch so that's material for another issue). I used the function in a blinky example and it built fine, flashed the binary to my STM32F4 nucleo board and it runs but unfortunately I don't have a serial-to-usb at home so I couldn't really test it. Maybe someone can volunteer to try it?

Regarding the file names I got inspiration from similar already existing files but if the names aren't right please let me know.

I look forward to your comments.

0xc0170 · 2017-04-26T08:22:56Z

I used the function in a blinky example and it built fine, flashed the binary to my STM32F4 nucleo board and it runs but unfortunately I don't have a serial-to-usb at home so I couldn't really test it. Maybe someone can volunteer to try it?

@bcostm @LMESTM please review, and can you verify these changes?

0xc0170 · 2017-04-26T08:21:12Z

targets/TARGET_STM/serial_api.c

+    init_uart(obj);
+}
+
+#endif /* DEVICE_SERIAL */


please always add a newline at the end of the file

Can you add them?

Strange... I thought I did maybe something happened in one of the rebases

I think the github viewer might be ignoring the last line. If I click edit this file I can see line 123 being a blank line. Also on the file information on top it says "123 lines (111 sloc) 4.19 KB" but on the viewer lines are only numbered 1 to 122. Have you checked out the change and see if the line is actually there? I can't do it now at work otherwise it'll have to wait until I have a moment at home. But I think the line is there. Same happens with the header file, different line count obviously.

0xc0170 · 2017-04-26T08:31:25Z

targets/TARGET_STM/serial_api_hal.h

+
+extern UART_HandleTypeDef uart_handlers[];
+
+void init_uart(serial_t *obj);


would be good to have this function documented here (use doxygen comments as other API)

Absolutely right, I forgot that.

fmanno · 2017-04-26T20:28:43Z

@0xc0170 Pushed changes addressing your two comments

0xc0170 · 2017-05-02T11:27:33Z

@bcostm @LMESTM please review, and can you verify these changes?

any update?

0xc0170 · 2017-05-02T11:34:14Z

/morph test

LMESTM · 2017-05-02T12:03:11Z

@0xc0170 is there a test in mbed-os test suite checking various serial format ?

0xc0170 · 2017-05-02T12:07:19Z

@0xc0170 is there a test in mbed-os test suite checking various serial format ?

I am not aware of it yet, would need to be implemented. I was thinking about checking with analyzer that this changes do not introduce any regression.

LMESTM · 2017-05-02T12:12:40Z

@0xc0170 If I need to make sure there is no regression on all STM32 targets, I'd rather use an automated test than checking manually :-)

0xc0170 · 2017-05-02T13:47:24Z

@0xc0170 If I need to make sure there is no regression on all STM32 targets, I'd rather use an automated test than checking manually :-)

that would be the best. @fmanno Are you familiar with host tests? Here is a snippet for one host test that is used in some tests: https://github.com/ARMmbed/mbed-os/blob/master/TESTS/host_tests/timing_drift_auto.py.
I imagine having host test as python serial module supports parity settings to verify the parity bits are correctly set.

@LMESTM what do you think?

LMESTM · 2017-05-02T13:57:40Z

Having a test with host and device using a few formats to be tested sounds good indeed. I remember there was such a test in MBED2 at least, testing a different baudrate (115200kbps) than the default one (9600). That is the exact same idea. Do we have this test as well in MEBD5?

LMESTM · 2017-05-19T07:34:50Z

@0xc0170 @adbridge @fmanno in order to progress on this item, couldn't we move on with this PR and get it merged, and open another issue which requires a new test case in MBED ?

0xc0170 · 2017-05-19T07:48:54Z

Please resolve the conflicts here

@0xc0170 @adbridge @fmanno in order to progress on this item, couldn't we move on with this PR and get it merged, and open another issue which requires a new test case in MBED ?

I would say yes as it was already tested with number of devices, and no regression found. We used to had one HAL serial test where we tested parity (2 uarts connected , no host test part needed in that case.).

LMESTM · 2017-05-19T08:00:49Z

I created #4348 to track the new test case creation

adbridge · 2017-05-19T10:16:32Z

@fmanno @LMESTM Normal policy would be to not accept this kind of change without the associated test case so that we can add that to our CI and ensure we have testing covered going forward. However this one has been hanging around for quite a while now so I think we can get it in as long as the test is forthcoming relatively soon :)

Although looks like a rebase is now required!

LMESTM · 2017-05-19T11:57:22Z

@adbridge I would argue that the MBED HAL already provides this feature from very long time ....

$ git commit 5c20760
Author: Emilio Monti [email protected]
Date: Mon Feb 18 15:32:11 2013 +0000

we're just fixing an issue for STM targets here, not introducing the feature.
The test should have been here already.

adbridge · 2017-05-19T13:12:00Z

@LMESTM You're right there should have already been a basic test for this functionality which could have then just been updated to make sure it tested this change as well. There are still some areas where we know the test coverage needs to improve. Unfortunately this seems to be one of them, so currently we have no way of testing if this all works correctly and thus we appreciate the addition of tests as it improves the quality for everyone :)

0xc0170 · 2017-05-26T12:37:40Z

There's a conflict, please rebase @fmanno

fmanno · 2017-05-26T19:15:47Z

I've now rebased it. Sorry it took me a bit long.

0xc0170 · 2017-05-30T13:24:47Z

oh, another conflict

fmanno · 2017-05-30T14:20:53Z

I'll try to get it tonight.

Reworked the serial_format() function for STM32F0x devices to take the format in the form: data_bits - parity - stop_bits E.g. 8 - N - 1 where data_bits exclude the parity bit. Added a case for 7 bits data as at least the chips STM32F0x1/STM32F0x2/STM32F0x8 support 7 bits data. Consolidated serial_format() and uart_init() functions into a general TARGET_STM serial_api.c file since the functions are common to all STM targets. Fixes ARMmbed#4189

0xc0170 · 2017-06-05T10:44:18Z

/morph test

mbed-bot · 2017-06-05T14:07:55Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 443

All builds and test passed!

sg- · 2017-06-05T21:28:46Z

targets/TARGET_STM/serial_api_hal.h

+}
+#endif
+
+#endif


Needs a newline

0xc0170 · 2017-06-06T10:25:12Z

Reviewed locally, the newlines are there.

fmanno mentioned this pull request Apr 22, 2017

STM32s Serial does not properly handle parity bits. #4189

Closed

0xc0170 requested a review from bcostm April 24, 2017 07:21

0xc0170 added the needs: review label Apr 24, 2017

LMESTM approved these changes Apr 24, 2017

View reviewed changes

0xc0170 approved these changes Apr 24, 2017

View reviewed changes

bcostm approved these changes Apr 24, 2017

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Apr 24, 2017

fmanno force-pushed the issue-4189 branch 2 times, most recently from 23cd698 to 9f779f4 Compare April 25, 2017 23:38

0xc0170 reviewed Apr 26, 2017

View reviewed changes

fmanno force-pushed the issue-4189 branch from 9f779f4 to fdcc3ad Compare April 26, 2017 20:27

LMESTM mentioned this pull request May 19, 2017

Need a MBED test for serial.format() settings like parity #4348

Closed

fmanno force-pushed the issue-4189 branch from fdcc3ad to 2384c05 Compare May 26, 2017 19:14

fmanno force-pushed the issue-4189 branch from 2384c05 to 5e98da1 Compare May 30, 2017 16:17

0xc0170 added needs: CI and removed needs: work labels Jun 5, 2017

sg- added ready for merge and removed needs: CI labels Jun 5, 2017

sg- reviewed Jun 5, 2017

View reviewed changes

targets/TARGET_STM/serial_api_hal.h

}

#endif

#endif

Copy link

Contributor

sg- Jun 5, 2017

Choose a reason for hiding this comment

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

Needs a newline

sg- added needs: work and removed ready for merge labels Jun 5, 2017

0xc0170 added ready for merge and removed needs: work labels Jun 6, 2017

sg- added the release-version: 5.5.0-rc2 label Jun 6, 2017

sg- merged commit 8232afa into ARMmbed:master Jun 7, 2017

sg- removed the ready for merge label Jun 7, 2017

fmanno deleted the issue-4189 branch June 23, 2017 11:21


		extern UART_HandleTypeDef uart_handlers[];

		void init_uart(serial_t *obj);

+              }
+              #endif
+              #endif

STM32s Serial: Correct handling of parity bits #4216

STM32s Serial: Correct handling of parity bits #4216

Uh oh!

Conversation

fmanno commented Apr 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Status

Migrations

Uh oh!

LMESTM left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Apr 24, 2017

Uh oh!

0xc0170 commented Apr 24, 2017

Uh oh!

bcostm commented Apr 24, 2017

Uh oh!

fmanno commented Apr 24, 2017

Uh oh!

mbed-bot commented Apr 24, 2017

Output

Uh oh!

0xc0170 commented Apr 25, 2017

Uh oh!

fmanno commented Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmanno commented Apr 25, 2017

Uh oh!

fmanno commented Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Apr 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmanno Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmanno commented Apr 26, 2017

Uh oh!

0xc0170 commented May 2, 2017

Uh oh!

0xc0170 commented May 2, 2017

Uh oh!

LMESTM commented May 2, 2017

Uh oh!

0xc0170 commented May 2, 2017

Uh oh!

LMESTM commented May 2, 2017

Uh oh!

0xc0170 commented May 2, 2017

Uh oh!

LMESTM commented May 2, 2017

Uh oh!

LMESTM commented May 19, 2017

Uh oh!

0xc0170 commented May 19, 2017

Uh oh!

LMESTM commented May 19, 2017

Uh oh!

adbridge commented May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LMESTM commented May 19, 2017

Uh oh!

adbridge commented May 19, 2017

fmanno commented Apr 22, 2017 •

edited

Loading

fmanno commented Apr 25, 2017 •

edited

Loading

fmanno commented Apr 25, 2017 •

edited

Loading

fmanno Jun 6, 2017 •

edited

Loading

adbridge commented May 19, 2017 •

edited

Loading