search

cvra / platform-abstraction

Platform abstraction layer for microcontrollers
3 stars 6 forks source link

Threading api with uCOS-III implementation #38

Closed msplr closed 10 years ago

msplr commented 10 years ago

The API is mostly based on what @antoinealb did in PR #23. The biggest difference is, that the thread's data structure os_thread_t can be statically allocated, which must be passed to os_thread_create() as an additional argument. (If I remember correctly, we discussed the malloc-topic already) It also implements newlib's malloc locking and switches newlib's context on every thread context switch. (This still needs to be tested)

Stapelzeiger commented 10 years ago

How about changing os_thread_sleep(int) to os_thread_sleep_ms(int)?

msplr commented 10 years ago

How about changing os_thread_sleep(int) to os_thread_sleep_ms(int)?

ok, sounds reasonable. will change

pierluca commented 10 years ago

How about changing os_thread_sleep(int) to os_thread_sleep_ms(int)?

hell yeah, +1

antoinealb commented 10 years ago

Looks good to me but I may not be very qualified to review that kind of patches...

antoinealb commented 10 years ago

Just something related to my field : you should add your files to package.yml :)

pierluca commented 10 years ago

I finally took the time to go through it and here are the main points for me:

1) how about breaking up OSTaskSwHook in several static, inline sub-functions? rationale: it's not very readable at the moment, overalls. By breaking it up you keep the same pay-for-what-you-use approach while making it much more digestible.

2) not clear and/or commented enough, examples:

3) (related) 0xFFFFFFF8 : where does this and other magic numbers come from?

4) (related, bis) why is OSTaskCreate call so well commented while OSTimeDlyHMSM has no documentation whatsoever as to the paremters' meaning? Try and do something similar or create local named variables so that the parameters usage isclear.

On Tue, Aug 26, 2014 at 10:43 PM, Antoine Albertelli < notifications@github.com> wrote:

Just something related to my field : you should add your files to package.yml :)

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

antoinealb commented 10 years ago

For point 4 HMSM stands for Hour Minute Second Millisecond. So it is pretty much self documenting I think.

pierluca commented 10 years ago

could you tell just by reading what this parameter means OS_OPT_TIME_HMSM_NON_STRICT ?

non-strict, is that "at least" that time or something else entirely ? I couldn't tell till checking the documentation.

I would strive to keep the code readable without having to look up APIs

On Thu, Aug 28, 2014 at 12:28 PM, Antoine Albertelli < notifications@github.com> wrote:

For point 4 HMSM stands for Hour Minute Second Millisecond. So it is pretty much self documenting I think.

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

msplr commented 10 years ago

1: ok, fixed it in last commit. Though I'm not sure if it really helps readability.

2: you're right, it needs more commenting. I just didn't focus on comments. I already fixed most of it.

3: yes, missing comment.

4: isn't os_thread_sleep_ms() saying what it does? I think it is wrong to explain OS_OPT_TIME_HMSM_NON_STRICT in the comment. In this case one should trust that the option is correct and otherwise one can check with the uCOS-III API.

Stapelzeiger commented 10 years ago

We should add a function that waits (sleep) at least a certain amount of time. Use case would be for example to wait for a sensor conversion to be completed before readout.

msplr commented 10 years ago

We should add a function that waits (sleep) at least a certain amount of time.

Done.

antoinealb commented 10 years ago 
/* unused hooks, need to be implemented though */

Does that mean we don't need them but we need a function implementation for the OS to build or does it mean we need them and you did not have the time to do it yet ?

msplr commented 10 years ago

It results in a linker error, because the OS is calling them. In the uCOS port example code they implemented them in the port and called function pointers set by the application during runtime.