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

Free serial resources if not needed anymore #10924

Merged
merged 14 commits into from
Oct 30, 2019

Conversation

ghseb
Copy link

@ghseb ghseb commented Jul 1, 2019

Description

As suggested this is an alternative to the proposal of #10843.

While in #10843 only UARTSerial was modified, this PR moves required changes into SerialBase.

Description of #10843:

If output and input is disabled for UARTSerial the pins should be reconfigured to archive lowest power consumption (e.g. to avoid current drain through TX pin).

With this change the UART interface is switched off if both input and output is disabled. If input or output is enabled again the UART pins are reinitialized.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[x] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

Release Notes

The handling of enable/disable input/output of UARTSerial is moved into SerialBase. The underlying serial peripheral is freed if both input and output are disabled to reduce current consumption. If either input or output is enabled again, the serial peripheral is reinitialized.

@ghseb
Copy link
Author

ghseb commented Jul 1, 2019

I tagged the proposal as Functionality change but im not sure if this is correct.

@ghseb
Copy link
Author

ghseb commented Jul 1, 2019

@kjbracey-arm I created an alternative proposal according to your suggestions.

@ciarmcom ciarmcom requested review from a team July 1, 2019 09:00
@ciarmcom
Copy link
Member

ciarmcom commented Jul 1, 2019

@ghseb, thank you for your changes.
@ARMmbed/mbed-os-core @ARMmbed/mbed-os-maintainers please review.

@kjbracey
Copy link
Contributor

kjbracey commented Jul 1, 2019

Thanks! On a 5-second skim, the structure looks promising.

Will review properly when I have time.

@artokin
Copy link
Contributor

artokin commented Jul 15, 2019

@ghseb , please check the failing CI results.

@SeppoTakalo
Copy link
Contributor

@kjbracey-arm If you have time, please review.

@SeppoTakalo
Copy link
Contributor

CI started.

@mbed-ci
Copy link

mbed-ci commented Jul 17, 2019

Test run: FAILED

Summary: 2 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests
  • jenkins-ci/mbed-os-ci_build-ARM

@SeppoTakalo
Copy link
Contributor

@ghseb Please fix those broken tests.

@SeppoTakalo
Copy link
Contributor

Unittests are failing:

[2019-07-17T11:48:10.263Z] /builds/ws/mbed-os-ci_unittests/unitTest-3443/mbed-os/UNITTESTS/stubs/SerialBase_stub.cpp: In constructor ‘mbed::SerialBase::SerialBase(PinName, PinName, int)’:

[2019-07-17T11:48:10.263Z] /builds/ws/mbed-os-ci_unittests/unitTest-3443/mbed-os/UNITTESTS/stubs/SerialBase_stub.cpp:22:1: error: uninitialized const member in ‘const enum PinName’ [-fpermissive]

[2019-07-17T11:48:10.263Z]  SerialBase::SerialBase(PinName tx, PinName rx, int baud)

[2019-07-17T11:48:10.263Z]  ^~~~~~~~~~

[2019-07-17T11:48:10.263Z] In file included from /builds/ws/mbed-os-ci_unittests/unitTest-3443/mbed-os/UNITTESTS/stubs/SerialBase_stub.cpp:18:0:

[2019-07-17T11:48:10.263Z] /builds/ws/mbed-os-ci_unittests/unitTest-3443/mbed-os/UNITTESTS/../drivers/SerialBase.h:333:22: note: ‘const PinName mbed::SerialBase::_tx_pin’ should be initialized

[2019-07-17T11:48:10.263Z]      const PinName    _tx_pin;

[2019-07-17T11:48:10.263Z]                       ^~~~~~~

[2019-07-17T11:48:10.263Z] /builds/ws/mbed-os-ci_unittests/unitTest-3443/mbed-os/UNITTESTS/stubs/SerialBase_stub.cpp:22:1: error: uninitialized const member in ‘const enum PinName’ [-fpermissive]

[2019-07-17T11:48:10.263Z]  SerialBase::SerialBase(PinName tx, PinName rx, int baud)

[2019-07-17T11:48:10.263Z]  ^~~~~~~~~~

[2019-07-17T11:48:10.263Z] In file included from /builds/ws/mbed-os-ci_unittests/unitTest-3443/mbed-os/UNITTESTS/stubs/SerialBase_stub.cpp:18:0:

[2019-07-17T11:48:10.263Z] /builds/ws/mbed-os-ci_unittests/unitTest-3443/mbed-os/UNITTESTS/../drivers/SerialBase.h:334:22: note: ‘const PinName mbed::SerialBase::_rx_pin’ should be initialized

[2019-07-17T11:48:10.263Z]      const PinName    _rx_pin;

[2019-07-17T11:48:10.263Z]                       ^~~~~~~

[2019-07-17T11:48:10.263Z] CMakeFiles/features-cellular-framework-AT-at_cellularcontext.dir/build.make:518: recipe for target 'CMakeFiles/features-cellular-framework-AT-at_cellularcontext.dir/stubs/SerialBase_stub.cpp.o' failed

[2019-07-17T11:48:10.263Z] make[2]: *** [CMakeFiles/features-cellular-framework-AT-at_cellularcontext.dir/stubs/SerialBase_stub.cpp.o] Error 1

[2019-07-17T11:48:10.263Z] CMakeFiles/Makefile2:1104: recipe for target 'CMakeFiles/features-cellular-framework-AT-at_cellularcontext.dir/all' failed

[2019-07-17T11:48:10.264Z] make[1]: *** [CMakeFiles/features-cellular-framework-AT-at_cellularcontext.dir/all] Error 2

[2019-07-17T11:48:10.264Z] Makefile:140: recipe for target 'all' failed

@SeppoTakalo
Copy link
Contributor

And ARM builds with:

[Error] @0,0: L6218E: Undefined symbol serial_free (referred from BUILD/tests/SDT32620B/ARM/drivers/SerialBase.o).

Copy link
Contributor

@kjbracey kjbracey left a comment

Choose a reason for hiding this comment

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

Okay, looks good, but want to promote enable_input to full public API of the base, and hence all serial classes.

drivers/SerialBase.h Outdated Show resolved Hide resolved
drivers/UARTSerial.cpp Outdated Show resolved Hide resolved
drivers/SerialBase.cpp Outdated Show resolved Hide resolved
@kjbracey
Copy link
Contributor

I'm away on holiday for a few weeks after today, so someone else needs to take over review here.

@kjbracey
Copy link
Contributor

This presumably would require a fix to #11057

@ghseb
Copy link
Author

ghseb commented Jul 23, 2019

And ARM builds with:

[Error] @0,0: L6218E: Undefined symbol serial_free (referred from BUILD/tests/SDT32620B/ARM/drivers/SerialBase.o).

I just added a stub to get it compiling because i don't know about that target (please inform me if this is a way to go).

@ghseb
Copy link
Author

ghseb commented Aug 8, 2019

During re-initialization probably flow control must also be considered to reestablish complete state.

@kjbracey
Copy link
Contributor

Missing serial_free should be fixed in the relevant target - eg see #11131 recently.

What do you think is required with respect to flow control?

I suppose we might want that the output line be put into "safe" state by serial_free - ie indicating "not ready to receive". But I'm not sure we should necessarily require that - I think a suspension of input is more "we don't care about input" rather than a "please hold!" order to the sender.

I can see an alternative argument for the line actually being left in "ready to receive" just to stop the sender getting jammed if we're not intending to ever read anything. Let it get thrown on the floor...

I admit I hadn't thought about this - I tend to forget flow control :) I was assuming input would just be binned, forgetting we maybe had the option of blocking the sender.

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 20, 2019

@ghseb Please review latest comments. I see you pushed an update after a review (please also comment if any of these address the review comments to have it linked and easily found).

What is left for this one to be integrated?

@ghseb
Copy link
Author

ghseb commented Aug 26, 2019

@kjbracey-arm What i ment is when flow control was enabled before serial is deinitialized, it should be re-enabled with same settings after serial is re-initialized again. This would require to backup the flow control state.

Regarding your thoughts for me it would be ok to leave it unspecified in which state hw flow control pins are left while serial input and output is disabled, because either way there might be potential issues that highly depend on the attached peripheral.

@kjbracey
Copy link
Contributor

kjbracey commented Aug 26, 2019

Right, yes, if you're using free and init to implement suspend/disable in SerialBase, then yes, SerialBase needs to preserve all its API settings that might get lost by free+init, including flow settings.

And yes, can leave pin states unspecified for now.

@mbed-ci
Copy link

mbed-ci commented Oct 28, 2019

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 8
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARM

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 29, 2019

I restarted the CI, will investigate if fails again

@mbed-ci
Copy link

mbed-ci commented Oct 29, 2019

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 9
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM

There is no Shutdown function for this target
@0xc0170
Copy link
Contributor

0xc0170 commented Oct 29, 2019

Fixed the build error, restarting CI

@mbed-ci
Copy link

mbed-ci commented Oct 29, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 10
Build artifacts

@0xc0170 0xc0170 merged commit 2203549 into ARMmbed:master Oct 30, 2019
@0xc0170
Copy link
Contributor

0xc0170 commented Oct 30, 2019

@ghseb Thanks for the contribution

@ghseb ghseb deleted the free-serial-resources-2 branch October 30, 2019 10:13
@0xc0170 0xc0170 added release-version: 5.15.0-rc1 and removed release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 labels Nov 19, 2019
mprse added a commit to mprse/mbed-os that referenced this pull request Dec 5, 2019
Related PR:

ARMmbed#10924

The above PR adds functions to disable/enable serial input/output. If both serial input and serial output are disabled, the peripheral is freed. If either serial input or serial output is re-enabled, the peripheral is reinitialized.

I missed this change while working on the static pinmap and unfortunately it has an impact on it. The reinitialization is a problem for static pinmap. Now the HAL init()/init_direct() function is called not only in the constructor (but also when re-enabling the peripheral). In the current version, even if static pinmap constructor was used to create an object (and init_direct() HAL API), when reinitialization is done it uses init() HAL API. This must be split.

If static pinmap constructor is used, then the peripheral must be always initialized using HAL init_direct() function. If regular the constructor is used, then the peripheral must be initialized using HAL init() function. The same split also must be done while setting flow control during reinitialization.
mprse added a commit to mprse/mbed-os that referenced this pull request Dec 5, 2019
Related PR:

ARMmbed#10924

The above PR adds functions to disable/enable serial input/output. If both serial input and serial output are disabled, the peripheral is freed. If either serial input or serial output is re-enabled, the peripheral is reinitialized.

I missed this change while working on the static pinmap and unfortunately it has an impact on it. The reinitialization is a problem for static pinmap. Now the HAL init()/init_direct() function is called not only in the constructor (but also when re-enabling the peripheral). In the current version, even if static pinmap constructor was used to create an object (and init_direct() HAL API), when reinitialization is done it uses init() HAL API. This must be split.

If static pinmap constructor is used, then the peripheral must be always initialized using HAL init_direct() function. If regular the constructor is used, then the peripheral must be initialized using HAL init() function. The same split also must be done while setting flow control during reinitialization.
mprse added a commit to mprse/mbed-os that referenced this pull request Dec 6, 2019
Related PR:

ARMmbed#10924

The above PR adds functions to disable/enable serial input/output. If both serial input and serial output are disabled, the peripheral is freed. If either serial input or serial output is re-enabled, the peripheral is reinitialized.

I missed this change while working on the static pinmap and unfortunately it has an impact on it. The reinitialization is a problem for static pinmap. Now the HAL init()/init_direct() function is called not only in the constructor (but also when re-enabling the peripheral). In the current version, even if static pinmap constructor was used to create an object (and init_direct() HAL API), when reinitialization is done it uses init() HAL API. This must be split.

If static pinmap constructor is used, then the peripheral must be always initialized using HAL init_direct() function. If regular the constructor is used, then the peripheral must be initialized using HAL init() function. The same split also must be done while setting flow control during reinitialization.
mprse added a commit to mprse/mbed-os that referenced this pull request Dec 9, 2019
Related PR:

ARMmbed#10924

The above PR adds functions to disable/enable serial input/output. If both serial input and serial output are disabled, the peripheral is freed. If either serial input or serial output is re-enabled, the peripheral is reinitialized.

I missed this change while working on the static pinmap and unfortunately it has an impact on it. The reinitialization is a problem for static pinmap. Now the HAL init()/init_direct() function is called not only in the constructor (but also when re-enabling the peripheral). In the current version, even if static pinmap constructor was used to create an object (and init_direct() HAL API), when reinitialization is done it uses init() HAL API. This must be split.

If static pinmap constructor is used, then the peripheral must be always initialized using HAL init_direct() function. If regular the constructor is used, then the peripheral must be initialized using HAL init() function. The same split also must be done while setting flow control during reinitialization.
mprse added a commit to mprse/mbed-os that referenced this pull request Dec 9, 2019
Related PR:

ARMmbed#10924

The above PR adds functions to disable/enable serial input/output. If both serial input and serial output are disabled, the peripheral is freed. If either serial input or serial output is re-enabled, the peripheral is reinitialized.

I missed this change while working on the static pinmap and unfortunately it has an impact on it. The reinitialization is a problem for static pinmap. Now the HAL init()/init_direct() function is called not only in the constructor (but also when re-enabling the peripheral). In the current version, even if static pinmap constructor was used to create an object (and init_direct() HAL API), when reinitialization is done it uses init() HAL API. This must be split.

If static pinmap constructor is used, then the peripheral must be always initialized using HAL init_direct() function. If regular the constructor is used, then the peripheral must be initialized using HAL init() function. The same split also must be done while setting flow control during reinitialization.
mprse added a commit to mprse/mbed-os that referenced this pull request Dec 9, 2019
Related PR:

ARMmbed#10924

The above PR adds functions to disable/enable serial input/output. If both serial input and serial output are disabled, the peripheral is freed. If either serial input or serial output is re-enabled, the peripheral is reinitialized.

I missed this change while working on the static pinmap and unfortunately it has an impact on it. The reinitialization is a problem for static pinmap. Now the HAL init()/init_direct() function is called not only in the constructor (but also when re-enabling the peripheral). In the current version, even if static pinmap constructor was used to create an object (and init_direct() HAL API), when reinitialization is done it uses init() HAL API. This must be split.

If static pinmap constructor is used, then the peripheral must be always initialized using HAL init_direct() function. If regular the constructor is used, then the peripheral must be initialized using HAL init() function. The same split also must be done while setting flow control during reinitialization.
adbridge pushed a commit that referenced this pull request Dec 11, 2019
Related PR:

#10924

The above PR adds functions to disable/enable serial input/output. If both serial input and serial output are disabled, the peripheral is freed. If either serial input or serial output is re-enabled, the peripheral is reinitialized.

I missed this change while working on the static pinmap and unfortunately it has an impact on it. The reinitialization is a problem for static pinmap. Now the HAL init()/init_direct() function is called not only in the constructor (but also when re-enabling the peripheral). In the current version, even if static pinmap constructor was used to create an object (and init_direct() HAL API), when reinitialization is done it uses init() HAL API. This must be split.

If static pinmap constructor is used, then the peripheral must be always initialized using HAL init_direct() function. If regular the constructor is used, then the peripheral must be initialized using HAL init() function. The same split also must be done while setting flow control during reinitialization.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet