pstolarz / OneWireNg

Arduino 1-wire service library. OneWire compatible. Dallas thermometers support.
BSD 2-Clause "Simplified" License
89 stars 20 forks source link

Add feature to skip id, when only one device on the bus available #55

Closed uzi18 closed 2 years ago

uzi18 commented 2 years ago

Please add also this common feature.

https://github.com/pstolarz/OneWireNg/blob/7cb5d9ae929b1b4ddbfca22fde840be349d9fcd3/src/drivers/DSTherm.cpp#L28-L51

pstolarz commented 2 years ago

Do you mean DSTherm::readScratchpad() variant like this one?

OneWireNg::ErrorCode readScratchpad(Scratchpad *scratchpad)
{
    OneWireNg::Id id;
    OneWireNg::ErrorCode ec;

    ec = _ow.readSingleId(id);
    if (ec == OneWireNg::EC_SUCCESS) {
        ec = readScratchpad(id, scratchpad);
    }
    return ec;
}
uzi18 commented 2 years ago

@pstolarz exactly.

pstolarz commented 2 years ago

The routine has 1 performance flaw. It needs to retrieve id of the connected sensor before communication with the sensor starts. In case of single-sensor environment the retrieval is simply redundant. For performance reason It's better to retrieve the connected sensor id once by calling readSingleId() and next use it afterwards with readScratchpad(OneWireNg::Id&, Scratchpad*). This way the performance is improved by eliminating the unnecessary calls. For this reason I'd not recommend to use the proposed routine in single-sensors environments. Do you still see any value of the routine?

pstolarz commented 2 years ago

I probably found other (better) solution of the problem for single-sensor setups - it's possible to use SKIP ROM command (0xCC) instead of MATCH ROM (0x55) and this way eliminate sending the id at all. Of course, the solution will only work on single-sensor environment since on multi-sensors setups this will cause transmission interference and will result with CRC error.

uzi18 commented 2 years ago

@pstolarz I see what You mean. In fact what I need is to simply issue commands like:

Something as simple as:

OneWireNg::ErrorCode readScratchpad(Scratchpad *scratchpad)
{
    OneWireNg::ErrorCode ec;

    ec = _ow.addressAll();
    if (ec == OneWireNg::EC_SUCCESS) {
                uint8_t cmd[1 + Scratchpad::LENGTH] = { 
             CMD_READ_SCRATCHPAD, 
             /* the read scratchpad will be placed here (9 bytes) */ 
             0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff 
         }; 

         _ow.touchBytes(cmd, sizeof(cmd)); 

         if (OneWireNg::crc8(&cmd[1], Scratchpad::LENGTH - 1) == 
             cmd[Scratchpad::LENGTH]) 
         { 
             new (scratchpad) Scratchpad(_ow, id, &cmd[1]); 
         } else 
             ec = OneWireNg::EC_CRC_ERROR; 
    }
    return ec;
}

What do you think?

uzi18 commented 2 years ago

code duplication could be resolved by reuse of small private method

pstolarz commented 2 years ago

That's exactly what I meant with my previous comment. That's solution is fine, I'll implement this. Thanks for the concept.

uzi18 commented 2 years ago

@pstolarz same for MAX31850 Idea is to enable simple solution for such case.

pstolarz commented 2 years ago

same for MAX31850

will be (almost) automatically by class inheritance.

I propose to name the new routine readScratchpadAll() to keep consistency with the existing functions: convertTempAll(), writeScratchpadAll(), copyScratchpadAll() etc. Beside this "All" postfix the routine is intended to be used only on single-sensor setups (which of course will be documented). What do you think?

uzi18 commented 2 years ago

@pstolarz ok, I don't remember how it is implemented in silicon, but it was something like "first" device will respond.

Simple example should be also available for Arduino people.

For example this problem in DallasTemperature lib was resolved by sequence: RESET,CMD_SEARCH_ROM,read rom,RESET,CMD_MATCH_ROM , CMD_READ_SCRATCHPAD

pstolarz commented 2 years ago

For example this problem in DallasTemperature lib was resolved by sequence: RESET,CMD_SEARCH_ROM,read rom,RESET,CMD_MATCH_ROM , CMD_READ_SCRATCHPAD

This sequence simply means - search ids and read them one-by-one. There is no improvement as in our case.

pstolarz commented 2 years ago

I don't remember how it is implemented in silicon, but it was something like "first" device will respond

All devices will respond, which means you get garbage on the bus if more than one sensor is connected.

uzi18 commented 2 years ago

For example this problem in DallasTemperature lib was resolved by sequence: RESET,CMD_SEARCH_ROM,read rom,RESET,CMD_MATCH_ROM , CMD_READ_SCRATCHPAD

This sequence simply means - search ids and read them one-by-one. There is no improvement as in our case.

They just use first found id, but this has also some overhead.

pstolarz commented 2 years ago

Last commit contains the enhancement in question. Unfortunately it was not possible to use the approach with SKIP ROM command, since the library need to know type of sensor to correctly interpret its scratchpad. For this reason its id is needed. But I found other mean to reduce unnecessary use of READ ROM command. Details in the new DSTherm::readScratchpadSingle() function in-line doc.