milesburton / Arduino-Temperature-Control-Library

Arduino Temperature Library
https://www.milesburton.com/w/index.php/Dallas_Temperature_Control_Library
974 stars 486 forks source link

setResolution() and other functions looping over devices inefficient and dangerous. #193

Open MalcolmBoura opened 3 years ago

MalcolmBoura commented 3 years ago

setResolution() and other functions looping by index exhibit O(n^2) duration.

They also send DS18 commands to every device on the bus, even those that are not DS18, which wastes time and may do something unexpected in consequence.

Steps To Reproduce Problem

Inspect the code.

Looping over all devices using for(i=0; i<devices; i++) { ... getAddress(); ...} is inherently inefficient because getAddress() carries out a linear search of the bus from the start for every call. This leads to O(n^2) behaviour.

Many functions are also dangerous because they apply DS18 commands to every device on the bus even those that are unsupported and may do something unexpected in consequence.

The fix is to provide an iterator, similar to that provided by OneWire, which iterates over only supported devices. It is both more efficient and cleaner code.

// start iterator over the supported devices
void DallasTemperature::iteratorReset() {    
    _wire->reset_search();
}

// continue iteration over the supported devices
bool DallasTemperature::iteratorNext(uint8_t* deviceAddress)  {
    while (_wire->search(deviceAddress)) {
        if (validAddress(deviceAddress) && validFamily(deviceAddress))
            return true;
    }
    return false;
}

The loop in setResolution() and similarly for similar loops in other functions, then becomes

        iteratorReset();
    while (iteratorNext(address)) {
        setResolution(address, bitResolution, true);
    }

NB the above has not yet been tested as I am engaged in some other more drastic changes. More on that in due course.

uzi18 commented 2 years ago

@pstolarz this should be resolved in OneWireNg ?