Clock API improvements

[hidmic]

This pull requests improves rcl_clock_* API documentation and adds missing error checks.

Note: to be rebased, not squashed, before merging so both commits are kept.

[dirk-thomas]

Can you clarify why the function isn't (same for the others)?

[hidmic]

I was unsure about this actually, as I have not found an explicit statement of what we mean by thread-safe here. I meant it in the strictest sense i.e. concurrent access to the same object (which is an argument purely because of a language limitation) is safe. If by thread-safe we mean safe concurrent access for different objects or with no regard for objects (i.e. no shared global state), then all these functions here are, including those that add and remove callbacks.

I wonder if Yes and No for values are enough. Maybe a third Conditional value would help us in being a bit more precise.

[dirk-thomas]

Maybe @wjwwood can clarify how the Thread-Safe flag is used throughout the existing API.

[clalancette]

I don't really think we need a third state for Thread-Safe. This one clearly isn't thread-safe; if you pass the same object into this method from two threads at the same time, that will leave the clock object in an undefined state. So it looks to me like "Thread-Safe: No" is appropriate here. The same goes for all of the APIs below.

[hidmic]

I thought the same, but it's also true that there're varying degrees of thread (un)safety. You can manage different clocks in different threads without issues. Functions that access global state don't even allow that.

[dirk-thomas]

And I think that kind of differentiated information would be very valuable for the user.

[hidmic]

@wjwwood bump

[wjwwood]

Sorry, I thought I replied to this, but maybe I was dreaming, I was talking about it with the Apex folks recently too.

The idea is that you can call it from multiple threads concurrently, but also generally that it isn't affected by other functions being called concurrently. If there's any threading considerations like "can be called concurrently but not on the same object" or "can be called concurrently but not concurrently with fini on the same object, etc...".

Basically, I consider the footnote to be more important than the Yes or No. If you say "Yes" without a note, that would mean to me that the function is most likely pure functional and has no global, or input/output, side effects. If it accesses global state or modifies an object passed by non-const pointer, then you should consider having a No or Yes with a note.

[hidmic]

Alright, so it could be either so long as there's a note. I'll add one.

[hidmic]

There, see ca78af8. It's a bit verbose but it is accurate.

[wjwwood]

Maybe we should make it clear elsewhere but the allocator calls are assumed to be thread safe. For now I think this can stay but it’s basically true everywhere we use an allocator but I don’t know if we mention it explicitly like this.

[hidmic]

Yeah, I don't think we're being this explicit elsewhere. To avoid so much repeat though, I wonder if we'd be better off documenting a basic thread-safety target for the library to attain e.g. all functions are reentrant but concurrent use of the same objects isn't safe, and then further qualifying it on a case-by-case basis e.g. the default allocator can be used concurrently, this function is fully thread-safe, this function is not reentrant, etc.

[hidmic]

Thanks for the reviews guys! Merging.