Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
MalcolmBoura opened this issue Feb 1, 2021 · 1 comment

Comments

@MalcolmBoura
Copy link

MalcolmBoura commented Feb 1, 2021

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.

Andersama added a commit to Andersama/Arduino-Temperature-Control-Library that referenced this issue Jun 11, 2022
As mentioned in milesburton#193 the issue stems from repeated usage of getAddress(). It doesn't seem to me to be necessary to use iterators to sort out the performance issue in the library, just a general avoidance of getAddress() outside index based functions. 

Since getAddress() appears to be used to do a bit more than iterate through addresses I added validAddress() as an additional condition, it might not be needed.

I only found two locations where this appears to happen so milesburton#193 may be resolved entirely by this. General advice to users of the library may be to avoid index based functions inside loops. This might be where an "iterator" interface may be useful inside the class, such that performing functions across devices remains somewhat linear.
@uzi18
Copy link

uzi18 commented Sep 14, 2022

@pstolarz this should be resolved in OneWireNg ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants