AM2320 returns default classes when read throw exceptions when accessed

[brianro]

Using Am2320.cs
When the read fails, the sensor class returns default (Temperature) default (Humidity) classes. The default classes cannot be differentiated from valid classes (different defect / suggestion posted in that library), so when the code accessed any of the class accessors, it throws a general type conversion exception. The core problem is a fix for the actual Temperature and humidity objects to not throw exceptions, but one hacky solution could be properties like HumidityError or TemperatureError or similar that return internal static values for comparison purposes e.g. if temperature is sensor.TemperatureError or humidity is sensor.HumidityError . Don't love either proposal but this is more related to the case where the values are read early in a process and then passed around, but you can't detect a failed read. that happened way earlier.

[josesimoes]

I've never used that binding and I took a quick look at the code. Shouldn't you check the IsLastReadSuccessful for this?

[Ellerbach]

+1. See: https://github.com/nanoframework/nanoFramework.IoT.Device/blob/a66a3ac088c00d9a259ad8f0f0be7dae40958869/devices/Am2320/samples/Program.cs#L33

[brianro]

On further examination, there is no bug but maybe a design paradigm issue.

The actual problem is a race condition can occur after reading the first property and IsLastReadSuccessful flag where reading the second property causes the IsLastReadSuccessful to actually change (see examples below) and this becomes messy in a loop when you are interested in multiple properties and have zero control of the "read".

part of the design puts excessive responsibility on the implementor to manage the multi-state relationship that creates a race condition between:

1. the flag, the property value, the hidden read method and the hidden timer.

These things makes it error prone in a loop:

1. IF you read one property before and one after the timer expires, the IsLastReadSuccessful can change between reading the properties. and you have to keep examining the flag.
2. Read is implicit with property access and hidden
3. There is no caching of the last valid temperature / humidity read

So the flag + implicit read + blocking timer, create a tri-state machine.

CODE Examples

This code is NOT stable and has to be re-written to function, see below. It causes a race condition in a loop. between good and bad values or reads. It is a simple use of the sensor and how I ran into the issue.

private static bool ReadSensor(Am2320 sensor, out Temperature temperature, out RelativeHumidity relativeHumidity)
{
 -- HERE IsLastReadSuccessful = false but timer not expired yet
temperature = sensor.Temperature;  -- HERE I get a DEFAULT(TEMPERATURE) - Broken!  timer NOT Expired
relativeHumidity = sensor.Humidity;  --- TRIGGERS READ - timer expires, VALUE is GOOD

return sensor.IsLastReadSuccessful;  -- RETURNS TRUE BUT temperature value is BAD.
}

This code is stable but tough to make efficient.

This code is stable but tough to make efficient.

private static bool ReadSensor(Am2320 sensor, out Temperature temperature, out RelativeHumidity humidity)
{
var readResult = sensor.IsLastReadSuccessful;

temperature = sensor.Temperature;
if (sensor.IsLastReadSuccessful != readResult ) return false;
relativeHumidity = sensor.Humidity;

}

Ideas for changes:

Let end external user control the read data method and return a bool result

eg.

bool readData() or bool readData(bool immediately)
or bool waitReadData();
or make readData() blocking.
or waitForData()

[Ellerbach]

Yes and no. The property is not updated in case there is no good read. So, you still have a proper value.
In case you are reading and indeed, there is a change and it's a valid value, you then may get the very last one.
In all cases, it's not recommended on any sensor like this to read too often the value.
And it only reads when it's outdated. See the code here: https://github.com/nanoframework/nanoFramework.IoT.Device/blob/a66a3ac088c00d9a259ad8f0f0be7dae40958869/devices/Am2320/Am2320.cs#L46

So the situation is actually not what you describe totally.

[brianro]

@Ellerbach The ternary with "(default)property) acts like an update as described. when there is a bad read It is actually a race condition The property is in a ternary based on the flag it is an implicit value substitution. For example return IsLastReadSuccessful ? GetTemperature() : default(Temperature); Returns the temp in the buffer OR " default()" even if the previous read was good. as described.

[josesimoes]

That binding is coming from IoT Core. I'm referring this to mention that I'm not aware of design discussions that happen at the time.
Having said that, I can't see a good reason for using that pattern with the timing to "prevent" a new reading each time that is requested (and instead provide the cached one).
The simplest approach would be to follow the usual pattern that we have in the majority of the bindings (and that follows the usual approach in the industry) which is perform a reading from the device by executing the respective I2C transaction.
That will take it's time (a few ms). On I2C failure it will throw an exception (we can swallow that and return an error flag). Integrity of the data can be checked with the CRC.

I would go for a rewrite, as you've suggested. Basically, simplifying it.