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

Backwards addresses #157

Closed mariusebastian closed 2 years ago

mariusebastian commented 4 years ago

I wonder if the addresses are handled correctly by this library.

I have connected 9pcs ds18b20 sensors to a wemos d1 mini. All devices are detected and when I print the addresses by index I get the following output:

Index Address
0 28 02 54 49 27 19 01 67
1 28 AA 48 00 58 14 01 8D
2 28 AA 68 08 58 14 01 69
3 28 AA 38 11 58 14 01 57
4 28 AA 8C 07 58 14 01 36
5 28 AA 36 09 58 14 01 D1
6 28 AA 2E 07 58 14 01 86
7 28 AA E7 00 58 14 01 D1
8 28 15 E4 3E 27 19 01 D8

When I set up tasmota on this device it detects the following addresses (one device is missing because tasmota only supports 8 devices, also the reason why I'm going back to this library):

Tasmota device id tasmota address corresponding index from the table above
DS18B20-1 01 14 58 00 48 AA 1
DS18B20-2 01 14 58 00 E7 AA 7
DS18B20-3 01 14 58 07 2E AA 6
DS18B20-4 01 14 58 07 8C AA 4
DS18B20-5 01 14 58 08 68 AA 2
DS18B20-6 01 14 58 09 36 AA 5
DS18B20-7 01 14 58 11 38 AA 3
DS18B20-8 01 19 27 49 54 02 0

The addresses seem to consist of the same bytes, but the order is reversed and the first and last byte of the address reported by this library is omitted by tasmota. I don't know which lib/fw is correct here, but with the tasmota way the indexes sort the addresses in rising order. With this library I'm not able to find the correspondence between index and address...

RobTillaart commented 4 years ago

Don't know the tasmota library. Do you have a link to its code?

mariusebastian commented 4 years ago

It's an open source firmware for esp8266 devices https://github.com/arendst/Tasmota

RobTillaart commented 4 years ago

Tasmota is a dedicated firmware for the ESP8266 with just too many files to dive into now (for me).

INDEX For the index to address part this Arduino-Temperature-Control-Library (ATCL) relies on the onewire library search function, so if the problem should be fixed, it should be fixed there.

The index is a way to identify individual sensors and there are realistic scenarios in which it just fails.

So it is in practice the index not a usable concept. The fact that Tasmota uses sorted addresses for index indicates that it also has the above two problems.

REVERSE ADDRESS Looking at https://datasheets.maximintegrated.com/en/ds/DS18B20.pdf especially figure 8 page 8 it shows the CRC in the MSB and the FamilyCode in the LSB.

If you assume MSB first ordering one could say that this ATCL library holds the address in the wrong order.

Also from page 8: The most significant 8 bits contain a cyclic redundancy check (CRC) byte that is calculated from the FIRST 56 bits of the ROM code.

The word FIRST indicates that the way this ATCL library represents the address is how the individual bytes are found in the ROM. Even if the ATCL library would be fairly easy too patch to use a reversed address, it is a major breaking change that would break many applications that use the addresses in the current "LSB first" way. Furthermore reversing the address bytes sec would not add or improve any functionality.

My conclusion So you are right in your observations [not verified] that there are differences between the two libraries regarding the index and the representation of the address. However I see no reason to change the ATCL and onewire library to reverse the address bytes, on contrary it would be a breaking change with no added value. Same for the index differences, even more as the index is a not usable addressing concept.

On the constructive side, I looked through the ATCL code and there are 6 spots that use "magic" numbers to indicate the CRC and the FamilyCode Bytes. Think these could be replaced in the ATCL code, something like

#define DSROM_FAMILY_CODE  0
#define DSROM_CRC  7
mariusebastian commented 4 years ago

Ok, thanks for looking into this. I can agree that this might not merit a breaking change, but it would be useful to know how the indexes are assigned. So since it's not sorted by the address as presented by the ATCL, what is it sorted by?

RobTillaart commented 4 years ago

Ok, thanks for looking into this. ....what is it sorted by?

Don't know if onewire::search() uses any sorting, the algorithm is not trivial. It uses a Search Rom [F0h] command and it evaluates bit by bit until 8 bytes are collected. At the end it copies the ROM bytes to the address.

See: https://github.com/PaulStoffregen/OneWire/blob/master/OneWire.cpp

A flowchart see datasheet figure 13

@milesburton please close this issue.

RobTillaart commented 4 years ago

@milesburton please close

gitkomodo commented 4 years ago

What ordering of ROM bytes is 'correct' is debatable. What I like about the 'tasmota-way' is that it uses most-significant-byte first AND most-significant-bit first. This library orders them least-significant-byte first and within those bytes most-significant-bit first, which can be confusing.

However, the OneWire library actually controls this ordering AND the algorithm used matches the description of Dallas/Maxim in their application note 187. As this is the ordering the manufacturer suggests, I'd say this is the 'correct' order.

The search algorithm also dictates the sorting of addresses. They're sorted by the least significant bit from 0 to 1. Example with 4 bits:

0100
0010
1001
0101
trycoon commented 4 years ago

I on the other hand wote for family code first. That's how the legendary OWFS package does it, and it has been working fine in 20 years or so. https://github.com/owfs/owfs-doc/wiki/DS18S20#addressing The good thing is that all devices are grouped in there family, so it's easier to spot if there is one DS18B20 missing out of the ten you got, instead of going thought the whole list of 30+ devices.

RobTillaart commented 4 years ago

idea four helper functions could easily convert one representation to another and get it printed (on Serial)

void tosmotaOrder(DeviceAddress addr) 
{
  if (addr[7] == 0x28 && addr[1] == 0x01) return; // already is tosmota order
  reverseAddress(addr);
}

void DSOrder(DeviceAddress addr) 
{
  if (addr[0] = 0x28 && addr[6] == 0x01) return; // already in Dallas order
  reverseAddress(addr);
}

void reverseAddress(DeviceAddress addr) 
{
  for (uint8_t i = 0, j = 7; i < 4; i++, j--)
  {
    uint8_t t = addr[i];
    addr[i] = addr[j];
    addr[j] = t;
  }
}

void printAddress(DeviceAddress addr, bool six = false )
{
  uint8_t start = 0, end = 8;
  if (six)
  {
    start = 1;
    end = 7;
  }
  for (uint8_t i = start; i < end; i++) 
  {
    if (addr[i] < 0x10) Serial.print('0');
    Serial.print(addr[i], HEX);
    Serial.print(' ');
  }
  // Serial.println();
}

(#Include not tested disclaimer)

RobTillaart commented 4 years ago

thinking out loud As 2 bytes are always the same and CRC can be calculated, one only need to store 5 bytes for unique address?