cvra / platform-abstraction

Platform abstraction layer for microcontrollers
3 stars 6 forks source link

Abstraction for mutexes. #5

Closed froj closed 10 years ago

antoinealb commented 10 years ago

I don't have much time to review right now, but the first thing I notice is a lot of duplicated mutex = os_mutex_create(); in your tests. It would be best to extract that into a setup method shared between tests ie :

void setup(void)
{
    mutex = os_mutex_create();
}

Just put it into your TEST_GROUP, next to the teardown method and it will be executed before each test.

antoinealb commented 10 years ago

After a more detailed review, here are my observations :

  1. Perhaps we should add an initial_value parameter to the mutex creation function to allow creation of a locked mutex.
  2. We should probably rework the documentation to avoid paraphrasing semaphore.h/mutex.h but instead provide an example (eventually of a test using a mutex). Also we would need to include an explanation of the difference between a binary semaphore and a mutex in uc/OS-III (priority promotion). This can be done in another pull request.

Globally looks good to merge once point 1 is sorted out, but given the current discussion about the semaphore API, I would like a review by someone else before merging.

froj commented 10 years ago
  1. Perhaps we should add an initial_value parameter to the mutex creation function to allow creation of a locked mutex.

I think it would be wrong to do that. A mutex

is used to gain exclusive access to a resource.

It doesn't make much sense to lock the resource at the moment when you create the mutex.

pierluca commented 10 years ago

agreed

On Thu, Jun 12, 2014 at 10:51 PM, froj notifications@github.com wrote:

  1. Perhaps we should add an initial_value parameter to the mutex creation function to allow creation of a locked mutex.

    I think it would be wrong to do that. A mutex

is used to gain exclusive access to a resource. It doesn't make much sense to lock the resource at the moment when you create the mutex.

— Reply to this email directly or view it on GitHub https://github.com/cvra/platform-abstraction/pull/5#issuecomment-45946322 .

antoinealb commented 10 years ago

Ok, then I merge :+1: