dallas: do not wait until value() to report errors

sensor loop assumes value(index) value is ok
ref. #2543

move all checks to pre(), value() returns pre-read value
- tick() and pre() are allowed to fail specific device
- pre() would fail the sensor if any of devices fail
- value(index) is never reached unless tick() and pre() succeed

code/espurna/sensors/DallasSensor.h

@@ -27,6 +27,9 @@
 #define DS_CMD_START_CONVERSION     0x44
 #define DS_CMD_READ_SCRATCHPAD      0xBE
 
+#define DS18x20_ADDR_LEN            8
+#define DS18x20_SCRATCHPAD_LEN      9
+
 // ====== DS2406 specific constants =======
 
 #define DS2406_CHANNEL_ACCESS 0xF5;
@@ -58,19 +61,20 @@
 // 1    0    CRC after 8 bytes
 // 1    1    CRC after 32 bytes
 #define DS2406_CHANNEL_CONTROL_BYTE 0x45;
-
 #define DS2406_STATE_BUF_LEN 7
 
 class DallasSensor : public BaseSensor {
 
     private:
 
-        using Address = std::array<uint8_t, 8>;
-        using Data = std::array<uint8_t, 9>;
+        using Address = std::array<uint8_t, DS18x20_ADDR_LEN>;
+        using Data = std::array<uint8_t, DS18x20_SCRATCHPAD_LEN>;
 
         struct Device {
             Address address;
             Data data;
+            uint8_t error{ SENSOR_ERROR_OK };
+            double value{ 0.0 };
         };
 
     public:
@@ -78,9 +82,8 @@ class DallasSensor : public BaseSensor {
         // ---------------------------------------------------------------------
 
         void setGPIO(unsigned char gpio) {
-            if (_gpio == gpio) return;
+            _dirty = _gpio != gpio;
             _gpio = gpio;
-            _dirty = true;
         }
 
         // ---------------------------------------------------------------------
@@ -158,77 +161,11 @@ class DallasSensor : public BaseSensor {
 
             // Every second we either start a conversion or read the scratchpad
             if (_conversion) {
-
-                // Start conversion
-                _wire->reset();
-                _wire->skip();
-                _wire->write(DS_CMD_START_CONVERSION, DS_PARASITE);
-
+                _startConversion();
             } else {
-
-                // Read scratchpads
-                for (size_t index = 0; index < _devices.size(); ++index) {
-
-                    if (_devices[index].address[0] == DS_CHIP_DS2406) {
-
-                        uint8_t data[DS2406_STATE_BUF_LEN];
-
-                        // Read scratchpad
-                        if (_wire->reset() == 0) {
-                            // Force a CRC check error
-                            _devices[index].data[0] = _devices[index].data[0] + 1;
-                            return;
-                        }
-
-                        _wire->select(_devices[index].address.data());
-
-                        data[0] = DS2406_CHANNEL_ACCESS;
-                        data[1] = DS2406_CHANNEL_CONTROL_BYTE;
-                        data[2] = 0xFF;
-
-                        _wire->write_bytes(data, 3);
-
-                        // 3 cmd bytes, 1 channel info byte, 1 0x00, 2 CRC16
-                        for(size_t i = 3; i < DS2406_STATE_BUF_LEN; ++i) {
-                            data[i] = _wire->read();
-                        }
-
-                        // Read scratchpad
-                        if (_wire->reset() == 0) {
-                            // Force a CRC check error
-                            _devices[index].data[0] = _devices[index].data[0] + 1;
-                            return;
-                        }
-
-                        memcpy(_devices[index].data.data(), data, DS2406_STATE_BUF_LEN);
-
-                    } else {
-
-                        // Read scratchpad
-                        if (_wire->reset() == 0) {
-                            // Force a CRC check error
-                            _devices[index].data[0] = _devices[index].data[0] + 1;
-                            return;
-                        }
-
-                        _wire->select(_devices[index].address.data());
-                        _wire->write(DS_CMD_READ_SCRATCHPAD);
-
-                        Data data{};
-                        for (size_t i = 0; i < data.size(); ++i) {
-                            data[i] = _wire->read();
-                        }
-
-                        if (_wire->reset() != 1) {
-                            // Force a CRC check error
-                            _devices[index].data[0] = _devices[index].data[0] + 1;
-                            return;
-                        }
-
-                        _devices[index].data = data;
-                    }
-                }
+                _readScratchpad();
             }
+
             _conversion = !_conversion;
         }
 
@@ -242,31 +179,28 @@ class DallasSensor : public BaseSensor {
 
         // Address of the device
         String address(unsigned char index) const override {
-            char buffer[20] = {0};
+            String out;
             if (index < _devices.size()) {
-                const auto& address = _devices[index].address;
-                snprintf_P(buffer, sizeof(buffer),
-                    PSTR("%02X%02X%02X%02X%02X%02X%02X%02X"),
-                    address[0], address[1], address[2], address[3],
-                    address[4], address[5], address[6], address[7]);
+                out = hexEncode(_devices[index].address);
             }
-            return String(buffer);
+
+            return out;
         }
 
         // Descriptive name of the slot # index
         String description(unsigned char index) const override {
-            char buffer[40];
+            String out;
             if (index < _devices.size()) {
-                const auto& address = _devices[index].address;
+                char buffer[64]{};
                 snprintf_P(buffer, sizeof(buffer),
-                    PSTR("%s (%02X%02X%02X%02X%02X%02X%02X%02X) @ GPIO%hhu"),
+                    PSTR("%s (%s) @ GPIO%hhu"),
                     chipAsString(index).c_str(),
-                    address[0], address[1], address[2], address[3],
-                    address[4], address[5], address[6], address[7],
-                    _gpio
-                );
+                    hexEncode(_devices[index].address).c_str(), _gpio);
+
+                out = buffer;
             }
-            return String(buffer);
+
+            return out;
         }
 
         // Type for slot # index
@@ -278,6 +212,7 @@ class DallasSensor : public BaseSensor {
                     return MAGNITUDE_TEMPERATURE;
                 }
             }
+
             return MAGNITUDE_NONE;
         }
 
@@ -288,91 +223,219 @@ class DallasSensor : public BaseSensor {
                 return 2;
             }
 
-            // In case we have DS2406, there is no decimal places
+            // In case we have DS2406, it is a digital sensor and there are no decimal places
             return 0;
         }
 
         // Pre-read hook (usually to populate registers with up-to-date data)
         void pre() override {
             _error = SENSOR_ERROR_OK;
+
+            for (auto& device : _devices) {
+                const auto chip_id = chip(device);
+
+                switch (chip_id) {
+                case DS_CHIP_DS2406:
+                    device.value = _valueDs2406(device.data);
+                    break;
+
+                case DS_CHIP_DS18S20:
+                    device.value = _valueDs18s20(device.data);
+                    break;
+
+                default:
+                    device.value = _valueGeneric(device.data);
+                    break;
+                }
+
+                if ((chip_id != DS_CHIP_DS2406) && (device.value == DS_DISCONNECTED)) {
+                    device.error = SENSOR_ERROR_OUT_OF_RANGE;
+                }
+
+                if (device.error != SENSOR_ERROR_OK) {
+                    DEBUG_MSG_P(PSTR("[DALLAS] %s @ GPIO%hhu (#%zu) reading failed\n"),
+                        _chipIdToString(chip(device)).c_str(), _gpio, index);
+                    _error = device.error;
+                    return;
+                }
+            }
         }
 
         // Current value for slot # index
         double value(unsigned char index) override {
+            if (index <= _devices.size()) {
+                return _devices[index].value;
+            }
+
+            return 0.0;
+        }
+
+    protected:
+
+        // ---------------------------------------------------------------------
+        // Protected
+        // ---------------------------------------------------------------------
+
+        // byte 0: temperature LSB
+        // byte 1: temperature MSB
+        // byte 2: high alarm temp
+        // byte 3: low alarm temp
+        // byte 4: DS18S20: store for crc
+        //         DS18B20 & DS1822: configuration register
+        // byte 5: internal use & crc
+        // byte 6: DS18S20: COUNT_REMAIN
+        //         DS18B20 & DS1822: store for crc
+        // byte 7: DS18S20: COUNT_PER_C
+        //         DS18B20 & DS1822: store for crc
+        // byte 8: SCRATCHPAD_CRC
+        static double _valueGeneric(const Data& data) {
+            int16_t raw = (data[1] << 8) | data[0];
+
+            const uint8_t res = ((data[4] & 0x60) >> 5) & 0b11;
+            switch (res) {
+            //  9 bit res, 93.75 ms
+            case 0x00:
+                raw = raw & ~7;
+                break;
+
+            // 10 bit res, 187.5 ms
+            case 0x20:
+                raw = raw & ~3;
+                break;
+
+            // 11 bit res, 375 ms
+            case 0x40:
+                raw = raw & ~1;
+                break;
+
+            // 12 bit res, 750 ms
+            default:
+                break;
 
-            if (index >= _devices.size()) {
-                return 0;
             }
 
-            const auto& data = _devices[index].data;
+            double out = raw;
+            raw /= 16.0;
 
-            if (chip(index) == DS_CHIP_DS2406) {
+            return out;
+        }
 
-                if (!OneWire::check_crc16(data.data(), 5, &data[5])) {
-                    _error = SENSOR_ERROR_CRC;
-                    return 0;
-                }
+        // See _valueGeneric(const Data&) for register info
+        static double _valueDs18s20(const Data& data) {
+            int16_t raw = (data[1] << 8) | data[0];
 
-                // 3 cmd bytes, 1 channel info byte, 1 0x00, 2 CRC16
-                // CHANNEL INFO BYTE
-                // Bit 7 : Supply Indication 0 = no supply
-                // Bit 6 : Number of Channels 0 = channel A only
-                // Bit 5 : PIO-B Activity Latch
-                // Bit 4 : PIO-A Activity Latch
-                // Bit 3 : PIO B Sensed Level
-                // Bit 2 : PIO A Sensed Level
-                // Bit 1 : PIO-B Channel Flip-Flop Q
-                // Bit 0 : PIO-A Channel Flip-Flop Q
-                return (data[3] & 0x04) !=  0;
+            // 9 bit resolution default
+            raw = raw << 3;
+
+            // "count remain" gives full 12 bit resolution
+            if (data[7] == 0x10) {
+                raw = (raw & 0xFFF0) + 12 - data[6];
             }
 
-            if (OneWire::crc8(data.data(), data.size() - 1) != data.back()) {
-                _error = SENSOR_ERROR_CRC;
-                return 0;
+            double out = raw;
+            out /= 16.0;
+
+            return out;
+        }
+
+        // 3 cmd bytes, 1 channel info byte, 1 0x00, 2 CRC16
+        // CHANNEL INFO BYTE
+        // Bit 7 : Supply Indication 0 = no supply
+        // Bit 6 : Number of Channels 0 = channel A only
+        // Bit 5 : PIO-B Activity Latch
+        // Bit 4 : PIO-A Activity Latch
+        // Bit 3 : PIO B Sensed Level
+        // Bit 2 : PIO A Sensed Level
+        // Bit 1 : PIO-B Channel Flip-Flop Q
+        // Bit 0 : PIO-A Channel Flip-Flop Q
+        static double _valueDs2406(const Data& data) {
+            return ((data[3] & 0x04) != 0) ? 1.0 : 0.0;
+        }
+
+        bool _readDs2406(Device& device) {
+            device.error = SENSOR_ERROR_OK;
+            if (_wire->reset() == 0) {
+                device.error = SENSOR_ERROR_TIMEOUT;
+                return false;
             }
 
-            // Registers
-            // byte 0: temperature LSB
-            // byte 1: temperature MSB
-            // byte 2: high alarm temp
-            // byte 3: low alarm temp
-            // byte 4: DS18S20: store for crc
-            //         DS18B20 & DS1822: configuration register
-            // byte 5: internal use & crc
-            // byte 6: DS18S20: COUNT_REMAIN
-            //         DS18B20 & DS1822: store for crc
-            // byte 7: DS18S20: COUNT_PER_C
-            //         DS18B20 & DS1822: store for crc
-            // byte 8: SCRATCHPAD_CRC
+            _wire->select(device.address.data());
 
-            int16_t raw = (data[1] << 8) | data[0];
-            if (chip(index) == DS_CHIP_DS18S20) {
-                raw = raw << 3;         // 9 bit resolution default
-                if (data[7] == 0x10) {
-                    raw = (raw & 0xFFF0) + 12 - data[6]; // "count remain" gives full 12 bit resolution
-                }
-            } else {
-                uint8_t cfg = (data[4] & 0x60);
-                if (cfg == 0x00) raw = raw & ~7;        //  9 bit res, 93.75 ms
-                else if (cfg == 0x20) raw = raw & ~3;   // 10 bit res, 187.5 ms
-                else if (cfg == 0x40) raw = raw & ~1;   // 11 bit res, 375 ms
-                                                        // 12 bit res, 750 ms
+            std::array<uint8_t, DS2406_STATE_BUF_LEN> data;
+            data[0] = DS2406_CHANNEL_ACCESS;
+            data[1] = DS2406_CHANNEL_CONTROL_BYTE;
+            data[2] = 0xFF;
+
+            _wire->write_bytes(data.data(), 3);
+
+            // 3 cmd bytes, 1 channel info byte, 1 0x00, 2 CRC16
+            _wire->read_bytes(data.data(), data.size());
+
+            // Read scratchpad
+            if (_wire->reset() == 0) {
+                device.error = SENSOR_ERROR_TIMEOUT;
+                return false;
             }
 
-            if ((raw & (int16_t) 0xfff0) == DS_DISCONNECTED) {
-                _error = SENSOR_ERROR_CRC;
-                return 0;
+            if (!OneWire::check_crc16(data.data(), 5, &data[5])) {
+                device.error = SENSOR_ERROR_CRC;
             }
 
-            return raw / 16.0;
+            static_assert(data.size() <= decltype(Device::data){}.size(), "");
+            std::copy(data.begin(), data.end(), device.data.begin());
 
+            return device.error == SENSOR_ERROR_OK;
         }
 
-    protected:
+        bool _readGeneric(Device& device) {
+            device.error = SENSOR_ERROR_OK;
+            if (_wire->reset() == 0) {
+                device.error = SENSOR_ERROR_TIMEOUT;
+                return false;
+            }
 
-        // ---------------------------------------------------------------------
-        // Protected
-        // ---------------------------------------------------------------------
+            _wire->select(device.address.data());
+            _wire->write(DS_CMD_READ_SCRATCHPAD);
+
+            Data data{};
+            _wire->read_bytes(data.data(), data.size());
+
+            if (_wire->reset() == 0) {
+                device.error = SENSOR_ERROR_TIMEOUT;
+                return false;
+            }
+
+            if (OneWire::crc8(data.data(), data.size() - 1) != data.back()) {
+                device.error = SENSOR_ERROR_CRC;
+            }
+
+            device.data = data;
+
+            return device.error == SENSOR_ERROR_OK;
+        }
+
+        bool _readScratchpad() {
+            for (size_t index = 0; index < _devices.size(); ++index) {
+                auto& device = _devices[index];
+
+                auto status =
+                    (device.address[0] == DS_CHIP_DS2406)
+                        ? _readDs2406(device)
+                        : _readGeneric(device);
+
+                if (!status) {
+                    return false;
+                }
+            }
+
+            return true;
+        }
+
+        void _startConversion() {
+            _wire->reset();
+            _wire->skip();
+            _wire->write(DS_CMD_START_CONVERSION, DS_PARASITE);
+        }
 
         static bool validateID(unsigned char id) {
             return (id == DS_CHIP_DS18S20)
@@ -382,37 +445,48 @@ class DallasSensor : public BaseSensor {
                 || (id == DS_CHIP_DS2406);
         }
 
-        String chipAsString(unsigned char index) const {
-            const char* ptr { nullptr };
+        static espurna::StringView _chipIdToStringView(unsigned char id) {
+            espurna::StringView out;
 
-            switch (chip(index)) {
+            switch (id) {
             case DS_CHIP_DS18S20:
-                ptr = PSTR("DS18S20");
+                out = STRING_VIEW("DS18S20");
                 break;
             case DS_CHIP_DS18B20:
-                ptr = PSTR("DS18B20");
+                out = STRING_VIEW("DS18B20");
                 break;
             case DS_CHIP_DS1822:
-                ptr = PSTR("DS1822");
+                out = STRING_VIEW("DS1822");
                 break;
             case DS_CHIP_DS1825:
-                ptr = PSTR("DS1825");
+                out = STRING_VIEW("DS1825");
                 break;
             case DS_CHIP_DS2406:
-                ptr = PSTR("DS2406");
+                out = STRING_VIEW("DS2406");
+                break;
+            default:
+                out = STRING_VIEW("Unknown");
                 break;
             }
 
-            if (!ptr) {
-                ptr = PSTR("Unknown");
-            }
+            return out;
+        }
+
+        static String _chipIdToString(unsigned char id) {
+            return _chipIdToStringView(id).toString();
+        }
+
+        String chipAsString(unsigned char index) const {
+            return _chipIdToString(chip(index));
+        }
 
-            return String(FPSTR(ptr));
+        static unsigned char chip(const Device& device) {
+            return device.address[0];
         }
 
         unsigned char chip(unsigned char index) const {
             if (index < _devices.size()) {
-                return _devices[index].address[0];
+                return chip(_devices[index]);
             }
 
             return 0;