Make mock of GPIO analog input pins more representative of the STM32 behaviour

[verityRF]

Summary: test_setAnalogPin only takes a singular integer as an input. For proper testing needs test_setAnalogPin needs an option to take in a deterministic sequence of integers that will be cycled through for each call. This is because sometimes the analogRead method is called multiple times internal to various controller modules and we need to be able to have it return different values each time it is called during unit/integrated testing.

Details: Example:

int[] myArray = {0, 1, 2};
int myArrayLength = 3;
Hal.test_setAnalogPin(A0, myArray, myArrayLength );
for (int i=0; i<6;  i++) {
printf(stdout, "%d, ", Hal.analogRead(A0));
}

would output:

0, 1, 2, 0, 1, 2, 

Relevant links: File In Question

[jlebar]

cc @jkff rtyi?

[jkff]

I thought about something similar - e.g. provide a callback instead of providing a value. Cycling through values of an array could be a special case of that.

However, it has a pretty big problem: the function stops being side-effect-free, e.g. the following two snippets stop being equivalent:

x = analogRead(A0);
if (x > 0) return x;

vs.

if (analogRead(A0) > 0) return analogRead(A0);

This may look like a silly example, but code with the same issues (but much harder to find) might appear through several layers of abstraction or through macros. Basically the consequence is that now a test that provides the values for the array needs to know a gritty implementation detail: exactly how many times is analogRead called, and in what order.

I was assuming that tests verifying multiple cycles of device operation would work like this:

TEST(Foo, Bar) {
  for (int cycle = 0; cycle < N; ++cycle) {
    ...prepare pin values for this cycle...
    device_cycle();
    ...assert that the device behaved correctly this cycle...
  }
}

@verityRF Do you think this would be sufficient?

[jlebar]

Now that I read this closely and understand...

This is exactly what gmock is for. Instead of having test_setAnalogPin, you make analogRead() a mock method, and then you can make it do whatever you want ON_CALL -- return the same value, return a series of values, whatever.

[jkff]

Yeah - but what I'm saying is, I think it's better to leave it as-is, rather than allow mocking it with arbitrary code, because that would encourage people to write tests overly coupled to the implementation and hence both hard to read and fragile, per my example. But it could be that I'm misunderstanding something here, so I'd like to first see an example that seems to require arbitrary mocking.

[jlebar]

I see now -- I totally agree that would be a too-tightly-coupled test strategy. analogRead should be side-effect-free.

Sorry for the noise.

[verityRF]

x = analogRead(A0);
if (x > 0) return x;

and

if (analogRead(A0) > 0) return analogRead(A0);

-should- have different behavior, as far as mimicking real controller behavior. In the current case, they are the same, but they shouldn't be.

The latter means two calls to analogRead, which are separate in time. A time varying real world signal is attached to A0, so if you call that method at time0, you'd get say 1.01 volts, then at time1, a couple instruction cycles later, could be 1.05 volts (depending on the sample rate of the ADC, conversion time, etc.)

I think having a more accurate way of being able to simulate the microcontroller analogRead is important. Yes, it requires the caller to have some idea of how the -test- method is implemented, but that's necessary to enable proper unit testing/integrated testing without hardware involved. This was the purpose of my ArduinoSim module beforehand, which allowed deterministic control over how the analogRead would be simulated from the test modules.

Again, some modules internally can have multiple calls to analogRead that cannot be exposed in the way you made an example for.

For instance in sensors.cpp

float getPressureReading(int pin, ...) {
float runningSum, conversionFactor = 0;
for(int i=0;i<numAvgSamples;i++) {
runningSum += analogRead(pin)*conversionfactor;
}
return runningSum/numAvgSamples;
}

In order to test averaging from an external unit test method, I need to preload analogRead via some test method to have different values. This would also hold true if we were to do digital filtering in place inside the loop.

Now of course, we could decouple the filtering and averaging into intermediate methods, and those can be unit tested individually, but at some point they have to be tied to a method that only contains an analogRead call (as it would be in the real controller code), so how do you do an integrated test on our native machines?

Options: 1) Having a more accurate analogRead mock for testing purposes 2) Just don't test anything that requires multiple analogRead calls internally, test only the algorithms that attach to them as separate methods by themselves 3) Include a bunch of preprocessor conditionals that get around this on a case-by-case basis (similar to what I was doing with ArduinoSim)

[jkff]

Thank you for the explanation! This makes sense, especially the getPressureReading example. I agree that this example cannot be adequately handled with the current test_setAnalogPin.

I'm still hesitant to pass an array though - in this example, this will require the caller to know numAvgSamples and to have extra code constructing exactly this many in their array; and we have to worry about potentially going past array bounds.

Could we pass a callback (std::function<int()>)? It would allow us to simulate noise here by giving a random number generator; if the caller insists on knowing the exact number of calls, they can declare an array themselves and give [&i]() { return my_array[i++] }.

[verityRF]

Right, I did have a way around this in ArduinoSim by just having an upper length fixed at compile time and the callers can reference that parameter, though that's not ideal either.

I don't actually care how it's done, wanted your ideas since you guys are more familiar with best practices and gmock itself. As long as we can prime it with a deterministic sequence of data then I'm happy, which I believe registering a callback would allow. This would allow the method to produce what is more reasonable and expected for real hardware operations from a test standpoint.

[verityRF]

Though I do have concern still about the original example

x = analogRead(A0);
if (x > 0) return x;

and

if (analogRead(A0) > 0) return analogRead(A0);

The fact that those would be the same in the default case for usage of analogRead during test runs concerns me. It sets up the precedent that someone who may not be thinking in a hardware perspective may think those being equivalent is guaranteed, especially since the HAL mock guarantees it, when in reality, it's never guaranteed.

I don't know what the fix for this might be, if we might even bother with it or just let it go. Would we enforce that you have to register a custom callback just to use it in the first place from the test perspective? That would cause headaches of its own.

[jkff]

I think having a custom callback be the only API should address your concern. Especially if the function has documentation that clearly explains the issue.

Note: anybody should feel free to take this issue. I'm gonna have less availability during the week during to day job.

[jlebar]

Is the issue just that we should be introducing noise into the analogRead calls? Like, instead of saying exactly what analogRead should return, the test harness says "returns a random number with mean X and variance Y" or something like that?

[verityRF]

The overlying issue is the lack of being able to preload a sequence of data. It could be noise, sure, but it could also be voltages corresponding to pressure waveforms for sensor module testing. It could be voltages corresponding to temperature data for other testing.

[jlebar]

I guess I am wondering if the following two things can be separated:

1. The desire to have a sequence of data, from
2. The desire to make analogRead() "realistic", addressing the concern in https://github.com/RespiraWorks/VentilatorSoftware/issues/137#issuecomment-616304690 that two subsequent calls to analogRead() should not necessarily return identical data.

I think Eugene and I are pushing back on the notion of "each time you call analogRead() you get a new value from a list" because we've seen this create extremely fragile tests and waste a lot of developer time. So I was wondering if you thought it would work to say, "up until this point in the test, all calls to analogRead return a noisy number near value X; then, after this point, all calls to analogRead return a noisy number near value Y".

[verityRF]

I understand the con, but I'm not sure that just having the callback only method causes that much issue, since the developer writing the test knows ahead of time how it would work since they wrote the stimuli. Sure it can cause headache, but I believe you gain from the more robust tests you can perform. It is a tradeoff between test writing concern and being able to do any sort of hardware out of the loop simulation. It is also a tradeoff between test coverage and how easy it is to write the test for certain functionalities.

If you two think this is really too bad of an idea, I'll relent. Do look at the attached sensors.cpp (and compare it to the live version in the repo), I want some feedback as to how to unit test the problem that required this discussion (you wouldn't be able to, you'd only depend on the averaging and filtering unit tests, but the zero_sensors() method couldn't be unit tested with how the HAL is written). sensors.txt

[jlebar]

Do look at the attached sensors.cpp (and compare it to the live version in the repo), I want some feedback as to how to unit test the problem that required this discussion (you wouldn't be able to, you'd only depend on the averaging and filtering unit tests, but the zero_sensors() method couldn't be unit tested with how the HAL is written).

What I'm seeing is that to unit test zero_sensors, we want to check that sensorZeroVals[pin] is the average of N readings from the pin.

I agree a callback (or even a list of sensor readings) would work for this case. When we're unit testing sensors.cpp, it's reasonable to expect we know exactly how many calls to analogRead will happen, because we are literally testing that we read from the pin N times.

I actually am not sure if we are in disagreement at this point. :) I think a callback as Eugene proposed or a proper gmockable function as I proposed would work fine? Like, they're both equivalent, in a sense.

[jkff]

Yeah I'm getting more and more convinced that a callback is the right thing here. As well as providing "write callbacks" for analogWrite and digitalWrite, because as I now understand, in the hardware world write(x);write(y) is not equivalent to write(y) either.

[jlebar]

Bug ikebana, I'm going to remove the "alpha" milestone because I don't think this blocks the alpha release. Which shouldn't stop anyone from working on this or anything.

[nimamoslemy]

Hi, we are a team that needs your feedback to complete our idea. Please visit our pages and comment on it. github gitlab

[jlebar]

@nimamoslemy hi, please see https://respira.works/contact if you'd like to engage with us. Bug trackers are for bugs.

[asmodai27]

The specific function mentioned in the title have been removed during HAL refactoring, but the point stands: we need a better mock for GPIO, especially analog input pin.