Closed bremoran closed 8 years ago
LGTM
@rgrover happy as a replacement for #23?
A side question - Shall we fix via separate PR - mbed namespace in this repository?
@rgrover Should be fixed in latest commit.
is irq_state the right name for these files? how about irq.[ch]? or interrupt.[ch]?
why do we have CriticalSectionLock and irq_state as separate concepts? what has been achieved here in addition to the already pending pull request: https://github.com/ARMmbed/core-util/pull/23
@rgrover This PR is explicitly to remove duplicated code. The CriticalSectionLock you proposed had code duplicated from minar-platform. irq_state moves the minar-platform code into a space that can be used by both minar and CriticalSectionLock.
Once this has been approved, the next step will be a patch to minar which uses the irq_state. Following that, I will push a minor revision to minar-platform which removes the irq_state APIs.
why don't we consolidate irq_state into CriticalSectionLock? why are they separate concepts? Isn't CriticalSectionLock the more generic concept?
@rgrover Feel free to file a PR for that.
@bremoran why are you introducing irq_state here? Why not leave this within CriticalSectionLock as is being suggested by pull #23?
+1 to consolidation.
@rgrover Please see my comment here: https://github.com/ARMmbed/core-util/pull/23#issuecomment-141401754
Are you planning to rewrite minar to use CriticalSectionLock?
@hugovincent -1 for consolidation. It doesn't make sense to me to force users into using an object oriented model that depends on object lifetimes.
@bremoran I'm not saying we should necessarily consolidate on the thing you don't like, I'm saying we shouldn't have two almost identical mechanisms with different semantics. It might be that to consolidate we have to build a third mechanism and then replace the first two.
@hugovincent I think you misunderstand me. I'm saying that the CriticalSectionLock
has some nice semantics for certain use-cases. I also can see how a procedural API has nice semantics for other use-cases. What I don't see is a good reason not to have both APIs available.
This should address the PR in https://github.com/ARMmbed/core-util/pull/23 is a more reusable way. Now, minar should be able to use its IRQ disable/enable logic from this source.