Open matthijskooijman opened 4 years ago
I think this might be straightforward to "phase in" to arduino_ci by simply having a "default" behavior that the user can override (the default being what it does currently). And then later, we can rename "default" to "legacy".
I'd suggest that this issue be considered blocked by https://github.com/ianfixes/arduino_ci/issues/146 since I'd prefer to avoid the case where fixing #146 breaks the fix for this issue (after much work has been done)
I think this might be straightforward to "phase in" to arduino_ci by simply having a "default" behavior that the user can override (the default being what it does currently). And then later, we can rename "default" to "legacy".
Yeah, sounds good.
I'd suggest that this issue be considered blocked by #146 since I'd prefer to avoid the case where fixing #146 breaks the fix for this issue (after much work has been done)
I'm not too worried about that, but let's fix #146 first anyway.
One thing I want to capture here explicitly (while I'm thinking about it) is the need to improve on "pin futures" https://github.com/Arduino-CI/arduino_ci/blob/master/REFERENCE.md#pin-futures
Right now you can queue up a series of bits to be read from a pin, but you can't tie this in any way to the concept of a signal. In other words, if you expect a square wave with a random period (let's say 456ms), there's no way to correlate the mock value being read to the mock millis()
.
Looking at the Arduino mocking code, I'm running into some things that are not entirely easy to implement right now. I spent some thought on this and am writing down some of these thoughts there. I don't have a very specific proposal yet when I start writing, so this might turn out to be a bit of a braindump style post, hopefully that will be helpful.
In particular, for my usecase I need:
millis()
/micros()
progression (e.g. one ms in the actual world should just advancemillis()
by one)Currently, the way to model behaviour of things like streams, time and pins is by setting variables that will then later be read by the mocking code. This means that you either need to pregenerate all data, or insert the data in a different thread (i.e. every ms update the counters, or whenever data comes in through a file, add it to
GODMODE()->serialPort[0].dataIn
, etc.). This is not quite ideal, and when you want to respond to certain events (e.g. whenever a byte is written, immediately reply or toggle a pin), there's no good way to do so (unless you continuously poll in a different thread, I think). I did see that PinHistory has some event/observer mechanism implemented, though I haven't looked at it in detail yet.A part of this issue has already been discussed in #136 (millis does not advance when delay is not called) and #135 (stream should not be mocked by default).
In general, it seems that it would be helpful if pretty much all of the mocked Arduino API would be able to:
These seem like they could be the same thing, but maybe they are separate (e.g. multiple handlers could respond to the same event, but only one of them actually decides the behavior, maybe?). Also, for unittests, it would be helpful if the behaviour would be easily controllable or switchable. E.g. that one unittest says "only advance time when delay is called", another says "just run against wall time" and a third says "ask me for the millis value every time".
I've been thinking a bit about how this could be modeled, and came up with the concept of a "Behavior" object. A mocked Arduino object, function or set of functions would just be a thin wrapper, that delegates most or all of its methods/function calls to an underlying behaviour object. Such a behaviour object is stored as a reference, so it can be easily replaced at runtime.
For example,
StreamBehaviour
could be an interface/abstract class, offeringwrite
andread
functions (basically the same as the virtual methods in the originalStream
class). A simple implementation could beStringStreamBehavior
, that just writes to / reads from two separate strings. A more elaborate implementation could be aFileStreamBehaviour
that reads / writes from a set of files or filedescriptors. A unit test could instantiate these behaviours and assign them to (e.g.) theSerial
objects where they need them (e.g.auto b = new StringStreamBehaviour(); Serial.setBehaviour(b); b.dataIn.append("foo")
).Note that for
Serial
, this probably means that theStream
class should be made identical to the original ArduinoStream
class (no mocking happening there), and then aMockedStream
or so subclass should be made, which accepts aStreamBehavior
. ThenHardwareSerial
could again be a subclass ofMockedStream
, which can additionally accept aHardwareSerialBehaviour
object for the extra things (e.g.Serial.begin()
/Serial.end()
). Maybe theHardwareSerialBehaviour
could include theStreamBehaviour
, or they could be set separately, dunno exactly.For things like
Serial
, the behaviour can be configured directly on the object itself (Serial.setBehaviour()
), but for things like pins that do not have a normal representation, or more abstract things like "timing" (that could controlmillis()
/micros()
/delay()
anddelayMicroseconds()
), these behaviors should probably be set on the godmode object or so.A completely different, more powerful, but also less convenient way of overriding object behaviour would be to really completely replace an object (e.g. replace
Serial1
by a custom subclass ofHardwareSerial
that defines some custom behaviour). This has the extra advantage that it can also actually add methods, rather then just replace behavior of predefined behavior, but it is also a lot more work and harder to dynamically change behavior. But maybe this is an option to consider in addition to swappable behaviors (something like-DOMIT_SERIAL1
and then allow the unit test to define its ownSerial1
object could already be sufficient).Out of time now, will followup later. In the meanwhile, thoughts as welcome!