search

ARMmbed / core-util

DEPRECATED: Mbed 3 utilities library
Other
12 stars 17 forks source link

use CriticalSectionLock in the implementation of atomic_cas #23

Closed rgrover closed 8 years ago

rgrover commented 8 years ago

@bogdanm @0xc0170 @bremoran

rgrover commented 8 years ago

passes testing against mbed-drivers using frdm-k64f-gcc.

rgrover commented 8 years ago

@0xc0170 @bremoran @bogdanm
I'm not happy about duplicating the implementation of pushDisableIRQ in CriticalSection. Perhaps there can be another work item to consolidate these implementations.

rgrover commented 8 years ago

tests for mbed-drivers pass for both k64f-gcc; I've also tested against minar-platform-posix. This is meant to generate a patch.

0xc0170 commented 8 years ago

I'm not happy about duplicating the implementation of pushDisableIRQ in CriticalSection. Perhaps there can be another work item to consolidate these implementations.

Is it time to have core-util + platform utils (we have this for minar.. minar/minar-platform/minar-platform-xxx). It's apparent that this type of abstraction (generic+ platform impl) will be more used in our code base.

I am not in favor of mbed-util/CriticalSectionLock.h implementation here - #ifdef TARGET_LIKE

rgrover commented 8 years ago

@0xc0170 @bremoran can we allow for this to go through at the moment and we can take up a separate task for splitting out core-util into platform-specific submodules? thanks.

0xc0170 commented 8 years ago

I would say yes, if we are quick to fix it :smile_cat: , what do you think @bremoran? I can merge this and release a new patch version.

rgrover commented 8 years ago

bump @bremoran Can we please get some traction for this?

bremoran commented 8 years ago

I don't like having this logic implemented in two places. I would have thought the correct implementation would be to build CriticalSectionLock on top of pushDisableIRQ; that was the original intention of those functions.

rgrover commented 8 years ago

@bremoran yes, we all agree that there is duplication. Can we handle that duplication as a separate task? pushDisableIRQ currently belongs to minar-platform, and needs to be moved into core-util. I agree that CriticalSectionLock should be implemented using pushDisableIRQ. Can we do that after we've moved it into core-util? and can we please move forward with this pull request for now? It is holding up other work which needs to go into core-util.

bremoran commented 8 years ago

What is your plan for implementing a C level critical section lock? And how does this apply to https://github.com/ARMmbed/core-util/pull/18

rgrover commented 8 years ago

Perhaps @hugovincent can fill in about our plan to implement a C-level CSection lock. Can we at least agree that this pull request is good to go ahead? It seems to me it has as much merit as your other pull request (#25) regarding irq_state.

bremoran commented 8 years ago

I don't like CriticalSectionLock. I find it confusing to use. It's got strange semantics when it's included inside another object and, overall, I find it an unnecessary use of object oriented programming where it does not need to exist.

I do not want to burden our users by forcing them to use an object oriented API which relies on creation and destruction of an object to implement something that could equally be done by function calls.

I don't mind including CriticalSectionLock, but only if it's not the only way to get a critical section. There needs to be a procedural API as well.

That being the case, CriticalSectionLock should be built on top of a procedural API.

rgrover commented 8 years ago

@bremoran can we separate these discussions? CriticalSectionLock has pre-existed this PR. This PR is about using CriticalSectionLock to hide platform specific implementations for masking interrupts. We can collectively decide a better abstraction for that independent of this PR.