There is a "System" namespace defined in the header file and a "system" namespace (lowercase) in the cpp file. The cpp file only adds complexity to the build process and should be removed, it is not needed at all; all functions can be defined in the header file. In order to define a standalone function in the header file, use the "inline" keyword - it has nothing to do with literal inlining in this case; rather, it informs the linker that the definition of this function may appear in other translation units (because it is included together with the header file).
On a related note, the new naming seems inconsistent - why the root namespace is named "chibios_rt" (lowercase), but the nested namespaces are CamelCased? I would suggest making all lowercase.
Declaring a function inline when it is defined in a different translation unit is pointless because normally the compiler won't be able to inline it anyway (unless some form of link-time optimization is used, although in that case, the compiler will probably ignore the inline keyword anyway).
I am noticing that the wrapper uses constructs from C++11 (e.g. "delete" keyword for special functions), but at the same time, it also makes use of deprecated entities like the NULL constant. Which C++ standard is it supposed to be compatible with? I strongly suggest to stick to C++11, in which case a couple of things will need updating.
Some methods of derived classes (e.g. BaseThread) return an instance of its base class by value. In C++ parlance this is known as "slicing" and, in general, it is extremely dangerous, as it essentially converts objects from one memory layout to another without a proper cast. Seeing as the sliced objects here do not contain any data entries defined in the derived types (unless I'm missing something), this is unlikely to cause any issues, but nevertheless, this is a poor practice. Such occurrences must return references, not values; furthermore, as C++ supports return type covariance, these should be references of the derived type, not the base type. Example:
Code: Select all
virtual ThreadReference start(tprio_t prio) {
void _thd_start(void *arg);
thread_ref = chThdCreateStatic(wa, sizeof(wa), prio, _thd_start, this);
return *this;
}
Correct implementation (the "virtual" keyword is redundant and should be replaced with "override", since we're using C++11 now anyway):
Code: Select all
BaseStaticThread<N>& start(tprio_t prio) override {
void _thd_start(void *arg);
thread_ref = chThdCreateStatic(wa, sizeof(wa), prio, _thd_start, this);
return *this;
}
The BaseThread class defines a virtual method main(), but since the application is supposed to redefine it, the default implementation is meaningless anyway. I suggest having it made pure virtual instead.
On the subject of const qualifiers - they are needed for typesafe API, but it's possible to cope without them (at the cost of somewhat less safe application code). A const-qualified method has its implicit "this" parameter const-qualified as well, which essentially means that such methods cannot modify the inner state of the object (barring some special circumstances). You can't make all of the methods const, obviously; I have skimmed through the wrapper and found only a handful of examples where this would make sense. For example:
Code: Select all
cnt_t getCounterI() {
return chSemGetCounterI(&sem);
}
A correct implementation must use const, because the inner state is not mutated:
Code: Select all
cnt_t getCounterI() const {
return chSemGetCounterI(&sem);
}
The above should work well. However, there may exist certain C API functions that are not const-correct, expecting a mutable reference to an object while not mutating its state by design. If that were the case here (it is not, because chSemGetCounterI is just a macro, but just for the sake of example), a hack would be required:
Code: Select all
cnt_t getCounterI() const {
return chSemGetCounterI(const_cast<semaphore_t*>(&sem));
}
I have attached an edited version of the wrapper based on this revision
https://github.com/ChibiOS/ChibiOS/blob ... ers/ch.hpp. I have not tested it, though. I wish I had time to fix everything, but there's too much on my plate already. I'd be happy to assist via the forum though!
C++ is a mess.
