Remove unintended Arduino dependencies from Runtime interface
platisd opened this issue · 12 comments
Description
The Runtime interface is supposed to be a platform agnostic representation of the
runtime environment.
Instead of passing arbitrary numbers (which will only work for Arduino), we should
instead pass generic enums and let each implementation determine how these
enums should be interpreted.
Definition of Done
- Platform specific dependencies are removed
While trying to add an ESP-IDF Runtime for this library (instead of forcing the Arduino one to be used), I've run into the Servo library being a runtime environment boundary; right now I can just conditionally polyfill it, but I was wondering if maybe you would rather simply abstract it away behind Runtime?
For me, the biggest problem begins with the fact that we use inheritance instead of composition for the sake of simplicity.
class ServoMotor : public Servo, public Motor
{
....
};But anyway, I should probably not rant about unrelated stuff... 😆
I don't know if I understood your suggestion correctly, but I don't think that Servo should be part of the Runtime interface as it is typically a separate library. How/Where are you getting stuck exactly?
The Smartcar library is dependent on some kind of Servo library. On most Arduino microcontrollers, this is the Servo library that comes with the IDE and for ESP32 is the ServoESP32 one.
Let's not ask me for suggestions on that first class problem else I'll end up proposing CRTP...
Well I am not using the Arduino Framework at all, which rules out that ServoESP32 library if I read its contents correctly (i.e. its PlatformIO config says that it only supports the Arduino Framework).
Let's not ask me for suggestions on that first class problem else I'll end up proposing CRTP...
😆
Well I am not using the Arduino Framework at all,
Cool, looking forward to see where this ends up.
From the top of my head the most straight forward way I see, is to create a Servo.h header available in the path and have a Servo implementation that provides the functionality in a similar API, i.e. with the Servo::attach and Servo::writeMicroseconds functions.
Thinking it twice, if we consider the "Servo functionality" as part of "Runtime" how would you propose we have it be considering that we need an instance of a "Servo" to have it working on Arduino?
That's exactly what I have right now: a polyfill for the two functions currently used by the library
If we choose to put the Servo stuff in the Runtime abstraction, given the need of a separate instance, a Servo factory as a method of Runtime could be a feasible solution.
Otherwise we simply make it a separate point of customization with an abstract base class like the rest of your design (you really gotta love the devirtualizer don't you?)
So what you propose is something like this (more or less pseudocode):
struct BasicServo {
virtual void writeMicroseconds() = 0;
virtual void attach(pin) = 0;
};
struct Runtime {
virtual BasicServo* getServo() = 0;
};
struct ServoMotor : public Motor {
ServoMotor(Runtime& runtime) : mServo{runtime.getServo()} {}
private:
BasicServo* mServo;
};And where in the ArduinoRuntime we would return a Servo, in EspIdfRuntime we would return a whatever the implementation is called?
I could consider it, especially since it will give us a single differentiation point between the different platforms, i.e. the Runtime interface, but will it make things otherwise easier?
Correct; that does cause the problem of ownership however (i.e. how does ServoMotor know how to correctly handle mServo on destruction?)
It does however allow one to use the library on any runtime with any Servo implementation that they want
Applying the same treatment to BasicServo as there is already to all other devices (i.e. the user passes in a ref to an child class instance) might have the added benefit of allowing user-customization of the Servo implementation without messing with the Runtime
Well, should we really care about destructing the servo instances? To me it does not physically make sense, considering the application domain we are in. In other words, if there is a Servo it is always there and so is "the runtime". As I see it, these classes don't get destructed.
Applying the same treatment to BasicServo as there is already to all other devices (i.e. the user passes in a ref to an child class instance) might have the added benefit of allowing user-customization of the Servo implementation without messing with the Runtime
Sure, that would be even better and more consistent design-wise, however, what would the benefit be with regards to the effort and in the number of differentiation points?
In both cases one would need to create a Servo implementation and there would be two context/classes to change/differentiate when migrating to a new platform.
In other words, if there is a Servo it is always there and so is "the runtime". As I see it, these classes don't get destructed.
If you only ever have a fixed number of servos, that'll definitely do; if not, well, you do have to release them. Note that the Arduino-provided <Servo.h> gives a MAX_SERVOS, but the ESP32 one you previously linked does not.
[...] what would the benefit be with regards to the effort and in the number of differentiation points?
In both cases one would need to create a Servo implementation and there would be two context/classes to change/differentiate when migrating to a new platform.
My point is that there is no difference in effort but one design allows direct user customization and the other simply doesn't so easily.
My point is that there is no difference in effort but one design allows direct user customization and the other simply doesn't so easily.
Then we are back to my initial rant then on preferring inheritance over composition. This was done to make it easier to use and because I could not find a good name for the BasicServo that would make sense. Perhaps a naming scheme such as ArduinoServo, Esp32Servo etc wouldn't be that bad and would even allow for a user to provide their own implementation without changing the library. Here I should note that having users provide their own types is not a common use-case for the current users of the library (i.e. university students) but perhaps as time passes it will become?
I'll keep this conclusion in mind and will adapt my current polyfilling. I'll get back to you once I'll have managed to successfully write my runtime impl if you're interested in the possible results.
Here I should note that having users provide their own types is not a common use-case for the current users of the library (i.e. university students) but perhaps as time passes it will become?
Well, your university students already use the ArduinoRuntime because it is a default parameter everywhere; I'd assume that Servo should get the same treatment and have ArduinoServo be an implicit default
Well, your university students already use the ArduinoRuntime because it is a default parameter everywhere; I'd assume that Servo should get the same treatment and have ArduinoServo be an implicit default
ArduinoRuntime as a default is honestly a hack that I am thinking of eventually getting rid off (maybe it's not that big of a deal to pass a Runtime instance around), but sure, that could be an idea! 😄