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

Add overridable wait hook #10693

Closed
wants to merge 1 commit into from
Closed

Add overridable wait hook #10693

wants to merge 1 commit into from

Conversation

geky
Copy link
Contributor

@geky geky commented May 28, 2019

Description

Added an overridable wait hook. Intended to match the semantics of mbed_override_console.

All you need to do is create a function named "mbed_override_wait_hook", and this function will be called anytime someone attempts to wait. This will let you run other code when a device would normally be wasting cycles.

Note that mbed_override_wait_hook is explicitly not called when we are in an interrupt (the reason for core_util_are_interrupts_enabled()). This is because interrupts violate a lot of the expectations of when you would be waiting, and interrupts really shouldn't be calling wait for anything other than small nanosecond delays.

Useful for external yield functions when there is no RTOS present, currently there's no other way to hook a function into when drivers are waiting for hardware.

Related: #9141

Pull request type

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

Reviewers

@kjbracey-arm, @0xc0170, @loverdeg-ep

Release Notes

Same as description?

@geky geky mentioned this pull request May 28, 2019
@ciarmcom ciarmcom requested review from 0xc0170, kjbracey and a team May 28, 2019 23:00
@ciarmcom
Copy link
Member

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

@incopeable

This comment has been minimized.

4 similar comments
@incopeable

This comment has been minimized.

@incopeable

This comment has been minimized.

@incopeable

This comment has been minimized.

@incopeable

This comment has been minimized.

@40Grit
Copy link

40Grit commented May 29, 2019

@kjbracey
Copy link
Contributor

Basic idea seems reasonable to me.

But note that #10104 significantly reworks the non-RTOS case, to the extent that spinning like this will be happening much less. You should be sleeping instead.

Also, having the hook in wait_us also is working a against #10609, which is desperately trying to reduce wait_us overhead. After that, wait_us can be super-tight with no function calls, and this would undo that unless there was some compile-time optimisation.

@kjbracey
Copy link
Contributor

After #10104 in particular, a common hook point becomes hal_sleep itself. That is the point at which Mbed OS says it has nothing to do, so if you want to waste time doing something else, you can do it there.

The spins should just be left for wait_us, which is often expected to be fast and high-precision, and thus maybe not suitable for this sort of call?

I've argued against hal_sleep hooks in the past, but that was from the point of view of "enter/exit" calls for drivers to do state-change stuff, but that was a separate argument about hal_sleep being too low-level for that concept, and what they really want is pre/post-suspend hooks.

But if it was "intercept and do something else rather than sleeping" - potentially the basis for a low-priority "thread" in non-RTOS, seems potentially useful.

@geky
Copy link
Contributor Author

geky commented May 29, 2019

Ah! I knew you were the right person to cc :) Sorry, I've been working through a long backlog and am not on top of all of the work that is going on.

We can just drop the hook from wait_us, from a second look it isn't productive. And only yielding in wait_ms matches the RTOS version.

I think you're right adding this to|after #10104 is the most appropriate, though this raises a few questions. I think I'll create a PR against your fork to ask them. It seems like the best places to hook into sleep are here and here (two because we only yield outside of critical sections)?

Sorry I'm late on all of this, it's probably past reasonable statute of limitations for me to critically review #10104, but it looks good! Does this mean we can deprecate PlatformMutex?

Deprecating wait() will be interesting.

@geky
Copy link
Contributor Author

geky commented May 29, 2019

Created PR PR here kjbracey#1

Useful for external yield functions, currently there's no other way to
hook a yield function into a driver
@geky
Copy link
Contributor Author

geky commented May 29, 2019

I've also updated this PR to be compatible with kjbracey#1, feel free to take either.

It's also out of wait_us now, although that required some duplication.

@kjbracey
Copy link
Contributor

Does this mean we can deprecate PlatformMutex?

Yes - Mutex now dummies itself bare metal.

it's probably past reasonable statute of limitations for me to critically review #10104

#10104 has just missed 5.13, so there is now time to look at it further before having it land, if you want to. Input would be appreciated.

@adbridge
Copy link
Contributor

@kjbracey-arm what is the status of this PR ?

@kjbracey
Copy link
Contributor

Needs work - discussion is happening over on my fork (although no progress in last couple of weeks).
I'd suggest parking this for now.

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 9, 2019

I'll close this , will revise later

cc @ARMmbed/mbed-os-core

@0xc0170 0xc0170 closed this Jul 9, 2019
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.

7 participants