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

Workaround to reset UARTE peripheral to be able to go into deep sleep #11779

Closed
wants to merge 0 commits into from
Closed

Workaround to reset UARTE peripheral to be able to go into deep sleep #11779

wants to merge 0 commits into from

Conversation

RobVlaar
Copy link

Description

Resolves: #11758

Pull request type

Added workaround to reset the UARTE peripheral on a destruction of SerialBase to be able to go into deep sleep.

Pull request type (required)

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results (required)

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers (optional)

@0xc0170

@ciarmcom ciarmcom requested review from 0xc0170 and a team October 31, 2019 08:00
@ciarmcom
Copy link
Member

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

@desmond-blue
Copy link
Contributor

Would you refer this fix?

kjbracey
kjbracey suggested changes Oct 31, 2019
View reviewed changes
Copy link
Contributor

@kjbracey kjbracey left a comment •
edited
Loading

Choose a reason for hiding this comment

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

In #10924 we shied away from adding serial_free to the destructor itself (for fear of breakage, using an untested HAL call), but similar changes are happening now in other classes - see #11746 - so I'm inclined to proceed with this now. (cc @mprse, @yarbcy, @bulislaw )

Just needs to be updated because of #10924

drivers/source/SerialBase.cpp Outdated
@@ -272,6 +272,7 @@ SerialBase::~SerialBase()
for (int irq = 0; irq < IrqCnt; irq++) {
attach(NULL, (IrqType)irq);
}
serial_free(&_serial);
Copy link
Contributor

Choose a reason for hiding this comment

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

#10924 has just being merged, and this change logically conflicts with that. You'll need to rebase and look again at the updated SerialBase.

After #10924, I believe this serial_free must be made conditional (on _rx_enabled || _tx_enabled, I think), and should be a call to _deinit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please rebase, this PR should contain just nrf52 fix - should be sufficient

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 31, 2019

Workaround to reset UARTE peripheral to be able to go into deep sleep #11779

If this is workaround, what is the real fix ? Using nordic driver did not fix the issue , did it?

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 31, 2019

The rebase resulted in the commits that should not be here, please review

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 31, 2019

@RobVlaar Your commit for nrf52 fix is also reverted. Can you create a branch from latest master and apply the patch there? You can create a new PR if that helps.

Or I might be able if I have permission to write to your branch to resolve this, let me know

@RobVlaar
Copy link
Author

@RobVlaar Your commit for nrf52 fix is also reverted. Can you create a branch from latest master and apply the patch there? You can create a new PR if that helps.

Or I might be able if I have permission to write to your branch to resolve this, let me know

yea something somewhere went terribly wrong, i'm still getting the hang of Github.
The commit is there now, but should i make a new PR or branch to clean up the other commits and reverts?

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 31, 2019

Clean-up here should be fine.

0xc0170
0xc0170 reviewed Oct 31, 2019
View reviewed changes
targets/TARGET_NORDIC/TARGET_NRF5x/TARGET_NRF52/serial_api.c Outdated

/* Turn NRF_UARTE0_BASE or NRF_UARTE1_BASE power off and on to reset peripheral. */
if (instance == 0) {
*(volatile uint32_t *)0x40002FFC = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

These write to registers do not have equivalent in their driver (more readable than writing to magic addresses) ?

Copy link
Author

Choose a reason for hiding this comment

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

I could use the NRF_UARTE0_BASE address and add FFC to it which is the power register which isn't defined by nordic for this peripheral. This should be something for nordic to fix.

@RobVlaar
Copy link
Author

@0xc0170 I have given you permission. I have tried a few thing to clean the current branch but im clueless right now, maybe you can help me out ?

@0xc0170 0xc0170 closed this Oct 31, 2019
@0xc0170
Copy link
Contributor

0xc0170 commented Oct 31, 2019

I was able to update to the latest master but failed to push that fix to your branch unfortunately , permission denied

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 31, 2019

I'll create a new PR, a sec

@0xc0170 0xc0170 mentioned this pull request Oct 31, 2019
Merged
@0xc0170
Copy link
Contributor

0xc0170 commented Oct 31, 2019

see #11790

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 left review comments

@kjbracey kjbracey kjbracey requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

using rawSerial adds about 600µA
5 participants
@RobVlaar @ciarmcom @desmond-blue @0xc0170 @kjbracey