ARMmbed / core-util

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

Added a c based re-entrant critical section API #88

Closed adbridge closed 8 years ago

adbridge commented 8 years ago

@bogdanm @bremoran

0xc0170 commented 8 years ago
#ifdef __cplusplus
namespace mbed {
namespace util {
extern "C" {
#endif
bogdanm commented 8 years ago

LGTM, but we also need to make the C++ implementation use the C implementation. Otherwise we'll end up with two parallel locking mechanisms, each with its own logic. This might seems out of scope for the original "create a C locking mechanism", but for the sake of consistency I believe we also need to modify the C++ implementation as part of this PR.

adbridge commented 8 years ago

@bogdanm Do we want to do that for the release? Bearing in mind that the c-based solution was added primarily to fix the https://github.com/ARMmbed/sal-driver-lwip-k64f-eth/issues/3 Although that actual fix should be very quick now we have an implementation.

bogdanm commented 8 years ago

Yes, that's what I was thinking too: it should be quite easy to re-write the C++ solution in terms of the C solution.

adbridge commented 8 years ago

@0xc0170 @bogdanm @bremoran review comments done

bogdanm commented 8 years ago

Why did you put critical.h under mbed-util instead of the existing core-util ?

adbridge commented 8 years ago

@bogdanm As that is where Brendan put it in his original proposal

bogdanm commented 8 years ago

Ah OK, that might've been before we renamed the repos. Please move it to core-util.

bremoran commented 8 years ago

Will this critical section work on NRF51? What about POSIX?

bremoran commented 8 years ago

Notwithstanding my previous comment, can we merge this first, then add support for POSIX and NRF51?

adbridge commented 8 years ago

@bogdanm @bremoran @0xc0170 additional comments done and re-tested, can we do a quick check and then merge.

rgrover commented 8 years ago

can you please add JIRA tickets for @pan- and I to complete testing against NRF51 and POSIX?

bremoran commented 8 years ago

LGTM

bogdanm commented 8 years ago

+1

bogdanm commented 8 years ago

Published new version 1.5.0