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.

Sign up for GitHub

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

Jump to bottom

STM32: check for UART ongoing transfers before entering deepsleep #7717

Merged
merged 2 commits into from
Aug 9, 2018
Merged

STM32: check for UART ongoing transfers before entering deepsleep #7717

merged 2 commits into from
Aug 9, 2018

Conversation

LMESTM
Copy link
Contributor

@LMESTM LMESTM commented Aug 7, 2018

Description

As suggested by @c1728p9 in mbed-os issue #7328, and until there is an
implementation of mbed-os issue #4408, we are implementing a workaround
at HAL level to check if there is any ongoing serial transfer (which happens
if HW FIFO is not yet empty).

In case a transfer is ongoing, we're not entering deep sleep and
return immediately.

Pull request type

[ x] Fix
[ ] Refactor
[ ] Target update
[ ] Feature
[ ] Breaking change

Tests results

+-------------------+---------------+--------------------------------------------+--------+--------------------+-------------+
| target | platform_name | test suite | result | elapsed_time (sec) | copy_method |
+-------------------+---------------+--------------------------------------------+--------+--------------------+-------------+
| NUCLEO_F091RC-ARM | NUCLEO_F091RC | tests-mbed_drivers-sleep_lock | OK | 18.45 | default |
| NUCLEO_F091RC-ARM | NUCLEO_F091RC | tests-mbed_hal-sleep | OK | 19.58 | default |
| NUCLEO_F091RC-ARM | NUCLEO_F091RC | tests-mbed_hal-sleep_manager | OK | 17.94 | default |
| NUCLEO_F091RC-ARM | NUCLEO_F091RC | tests-mbed_hal-sleep_manager_racecondition | OK | 31.01 | default |
| NUCLEO_F103RB-ARM | NUCLEO_F103RB | tests-mbed_drivers-sleep_lock | OK | 18.42 | default |
| NUCLEO_F103RB-ARM | NUCLEO_F103RB | tests-mbed_hal-sleep | OK | 18.28 | default |
| NUCLEO_F103RB-ARM | NUCLEO_F103RB | tests-mbed_hal-sleep_manager | OK | 18.17 | default |
| NUCLEO_F103RB-ARM | NUCLEO_F103RB | tests-mbed_hal-sleep_manager_racecondition | OK | 31.32 | default |
| NUCLEO_F207ZG-ARM | NUCLEO_F207ZG | tests-mbed_drivers-sleep_lock | OK | 16.96 | default |
| NUCLEO_F207ZG-ARM | NUCLEO_F207ZG | tests-mbed_hal-sleep | OK | 17.0 | default |
| NUCLEO_F207ZG-ARM | NUCLEO_F207ZG | tests-mbed_hal-sleep_manager | OK | 16.69 | default |
| NUCLEO_F207ZG-ARM | NUCLEO_F207ZG | tests-mbed_hal-sleep_manager_racecondition | OK | 29.78 | default |
| NUCLEO_F303ZE-ARM | NUCLEO_F303ZE | tests-mbed_drivers-sleep_lock | OK | 18.35 | default |
| NUCLEO_F303ZE-ARM | NUCLEO_F303ZE | tests-mbed_hal-sleep | OK | 19.59 | default |
| NUCLEO_F303ZE-ARM | NUCLEO_F303ZE | tests-mbed_hal-sleep_manager | OK | 17.97 | default |
| NUCLEO_F303ZE-ARM | NUCLEO_F303ZE | tests-mbed_hal-sleep_manager_racecondition | OK | 31.0 | default |
| NUCLEO_F446RE-ARM | NUCLEO_F446RE | tests-mbed_drivers-sleep_lock | OK | 17.47 | default |
| NUCLEO_F446RE-ARM | NUCLEO_F446RE | tests-mbed_hal-sleep | OK | 18.95 | default |
| NUCLEO_F446RE-ARM | NUCLEO_F446RE | tests-mbed_hal-sleep_manager | OK | 17.25 | default |
| NUCLEO_F446RE-ARM | NUCLEO_F446RE | tests-mbed_hal-sleep_manager_racecondition | OK | 30.03 | default |
| NUCLEO_F767ZI-ARM | NUCLEO_F767ZI | tests-mbed_drivers-sleep_lock | OK | 16.83 | default |
| NUCLEO_F767ZI-ARM | NUCLEO_F767ZI | tests-mbed_hal-sleep | OK | 18.33 | default |
| NUCLEO_F767ZI-ARM | NUCLEO_F767ZI | tests-mbed_hal-sleep_manager | OK | 16.58 | default |
| NUCLEO_F767ZI-ARM | NUCLEO_F767ZI | tests-mbed_hal-sleep_manager_racecondition | OK | 29.73 | default |
| NUCLEO_L073RZ-ARM | NUCLEO_L073RZ | tests-mbed_drivers-sleep_lock | OK | 20.06 | default |
| NUCLEO_L073RZ-ARM | NUCLEO_L073RZ | tests-mbed_hal-sleep | OK | 21.62 | default |
| NUCLEO_L073RZ-ARM | NUCLEO_L073RZ | tests-mbed_hal-sleep_manager | OK | 19.55 | default |
| NUCLEO_L073RZ-ARM | NUCLEO_L073RZ | tests-mbed_hal-sleep_manager_racecondition | OK | 33.32 | default |
| NUCLEO_L152RE-ARM | NUCLEO_L152RE | tests-mbed_drivers-sleep_lock | OK | 18.02 | default |
| NUCLEO_L152RE-ARM | NUCLEO_L152RE | tests-mbed_hal-sleep | OK | 19.9 | default |
| NUCLEO_L152RE-ARM | NUCLEO_L152RE | tests-mbed_hal-sleep_manager | OK | 17.99 | default |
| NUCLEO_L152RE-ARM | NUCLEO_L152RE | tests-mbed_hal-sleep_manager_racecondition | OK | 30.94 | default |
| NUCLEO_L476RG-ARM | NUCLEO_L476RG | tests-mbed_drivers-sleep_lock | OK | 17.44 | default |
| NUCLEO_L476RG-ARM | NUCLEO_L476RG | tests-mbed_hal-sleep | OK | 18.92 | default |
| NUCLEO_L476RG-ARM | NUCLEO_L476RG | tests-mbed_hal-sleep_manager | OK | 17.67 | default |
| NUCLEO_L476RG-ARM | NUCLEO_L476RG | tests-mbed_hal-sleep_manager_racecondition | OK | 30.09 | default |
+-------------------+---------------+--------------------------------------------+--------+--------------------+-------------+

@LMESTM
STM32: check for UART ongoing transfers before entering deepsleep
402f3f1 
As suggested by Russ Butler in mbed-os issue ARMmbed#7328, and until there is an
implementation of mbed-os issue ARMmbed#4408, we are implementing a workaround
at HAL level to check if there is any ongoing serial transfer (which happens
if HW FIFO is not yet empty).

In case a transfer is ongoing, we're not entering deep sleep and
return immediately.
@LMESTM
Copy link
Contributor Author

LMESTM commented Aug 7, 2018

@jeromecoutant @c1728p9

@0xc0170 0xc0170 requested a review from a team August 7, 2018 10:01
0xc0170
0xc0170 reviewed Aug 7, 2018
View reviewed changes
targets/TARGET_STM/serial_api.c Outdated
* yet. Returns 1 if there is at least 1 serial instance with ongoing ransfer
* 0 otherwise.
*/
int serial_IsTxOngoing(void) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall this follow the style, serial_is_tx_ongoing - assuming its in the HAL (I can see this is close to driver naming as LL_USART_IsEnabled())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right ! will do the change

@0xc0170 0xc0170 requested a review from c1728p9 August 7, 2018 11:26
c1728p9
c1728p9 reviewed Aug 7, 2018
View reviewed changes
targets/TARGET_STM/serial_api.c
int TxOngoing = 0;

#if defined(USART1_BASE)
if (LL_USART_IsEnabled(USART1) && !LL_USART_IsActiveFlag_TC(USART1)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When serial is first enabled is the TC flag set, or does a transfer actually have to complete for this to be set? If it isn't set then UARTs which are only read from will block deep sleep all the time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm ... Reset value of TC flag is 1. It is set to 0 in case there is a new TX byte or in case it is explicitly cleared by software - which could be the case in serial ASYNCH mode ...
So Maybe I need a specific logic for Asynch ... but for UARTs that are only read, that should be fine.

Extract from spec:

Interrupt and status register (USARTx_ISR)
Address offset: 0x1C
Reset value: 0x0200 00C0
[...]
Bit 6 TC: Transmission complete
This bit is set by hardware if the transmission of a frame containing data is complete and if
TXE is set. An interrupt is generated if TCIE=1 in the USARTx_CR1 register. It is cleared by
software, writing 1 to the TCCF in the USARTx_ICR register or by a write to the
USARTx_TDR register.
An interrupt is generated if TCIE=1 in the USARTx_CR1 register.
0: Transmission is not complete
1: Transmission is complete
Note:If TE bit is reset and no transmission is on going, the TC bit will be set immediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that in case of asynch I would expect the application or driver using this mode to be in charge of locking deep sleep until Tx is complete - don't you think ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was looking at the datasheet for the F429 which appears to have different behavior. The transfer complete check may need to be different for each family.

image

Bit 6 TC: Transmission complete
This bit is set by hardware if the transmission of a frame containing data is complete and if
TXE is set. An interrupt is generated if TCIE=1 in the USART_CR1 register. It is cleared by
a software sequence (a read from the USART_SR register followed by a write to the
USART_DR register). The TC bit can also be cleared by writing a '0' to it. This clearing
sequence is recommended only for multibuffer communication.
0: Transmission is not complete
1: Transmission is complete

Copy link
Contributor Author

@LMESTM LMESTM Aug 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I think you've got an old spec. In recent one (rev17), the reset value is 0xC0

https://www.st.com/content/ccc/resource/technical/document/reference_manual/3d/6d/5a/66/b4/99/40/d4/DM00031020.pdf/files/DM00031020.pdf/jcr:content/translations/en.DM00031020.pdf

Also I've just done tests (on NUCLEO_F429ZI) using Rx only on 1 UART while printing with another one and this works fine, i.e. Deep sleep is well entered and there is no garbage character at the end of the printed line (deep sleep is actually only entered after the transmission is complete).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah good catch, I had an old version of the datasheet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also had an old one ! ... so it took me some time and tests to figure that out :-)

@cmonr
Copy link
Contributor

cmonr commented Aug 8, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Aug 8, 2018

Build : SUCCESS

Build number : 2766
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7717/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Aug 8, 2018

Exporter Build : SUCCESS

Build number : 2395
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/7717/

@mbed-ci
Copy link

mbed-ci commented Aug 9, 2018

Test : SUCCESS

Build number : 2492
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/7717/2492

@cmonr cmonr merged commit e85acac into ARMmbed:master Aug 9, 2018
pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018
Merge pull request ARMmbed#7717 from LMESTM/fix_checkfifo
27f76f3 
STM32: check for UART ongoing transfers before entering deepsleep
@mattbrown015 mattbrown015 mentioned this pull request Oct 11, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeromecoutant jeromecoutant jeromecoutant approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

@c1728p9 c1728p9 c1728p9 approved these changes

Assignees
No one assigned
Labels
release-version: 5.9.5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@LMESTM @cmonr @mbed-ci @c1728p9 @0xc0170 @jeromecoutant