[OneWire] add EDS sensors

[J-N-K]

add EDS006x
partly implements #6177

• add code
• add tests
• confirm code works

Depends on #6626

Signed-off-by: Jan N. Klug jan.n.klug@rub.de

[J-N-K]

second part of #6350

depends on #6431

[kaikreuzer]

@J-N-K We plan to do the ESH 0.10.0 release next week - if there's a chance for you to finish this off by Monday, we should get it in.

[J-N-K]

I don't think so. I'm waiting for response from the owner of these devices if the implementation is working. I can't test that in my own setup.

[J-N-K]

We now have confirmation that the code itself works. I'll finish documentation and tasks after #6626 is merged as this will create merge conflicts otherwise.

[maggu2810]

some minor comments

[maggu2810]

Wouldn't it be much more readable to add another OwDeviceParameterMap constructor that takes a thing type and a OwserverDeviceParameter as an argument?

[J-N-K]

I'm still think of other bridge types which would require other device parameters (e.g. a bridge that directly uses the DS9490 USB Onewire-Master would need the register inside the device instead of a path). This would require something like

private final OwDeviceParameterMap temperatureParameter = new OwDeviceParameterMap() {
    {
        set(THING_TYPE_OWSERVER, new OwserverDeviceParameter("/temperature"));
        set(THING_TYPE_DS9490R, new Ds9490RDeviceParameter(0x4711));
    }

I don't think that can be done with constructors.

[J-N-K]

If we agree that this will never happen, the code could be refactored and simplified. But that is quite large (not sure if we would need a CQ for that).

[maggu2810]

Or we add a constructor:

public OwDeviceParameterMap(final Map<ThingTypeUID, OwDeviceParameter> map) {
    this.map.putAll(map);
}

Then you could add multiple key value pairs on construction or a single one.

private final OwDeviceParameterMap temperatureParameter = new OwDeviceParameterMap(Collections.singletonMap(THING_TYPE_OWSERVER, new OwserverDeviceParameter("/temperature")));

I don't understand the current implementation of the set method of OwDeviceParameterMap.
There is a check if the key is already contained to call replace or put.
Why not calling put without additional checks?

[J-N-K]

Is that really more readable for let's say three parameter sets? Looking at this discussion it seems that double-brace-method is most clear if we stay away from Guava and don't have Java 9 at hand.

Another argument not to change it now: This is the way it is for all other devices. As I said before: we could think of removing the map totally and giving up the idea of other bridges. With the coming changes in #6764 it 's becoming more and more difficult and would require a rewrite of large parts of the OWFS code to get the same functionality. But IMO this is out of scope of this PR.

Regarding the latter remark: I don't remember. Probably it wasn't a HashMap in a previous version during development. I have removed the check.

[maggu2810]

@J-N-K You can leave it as it is if you want.

But AFAIK using that code you create for every variable a new inner anonymous class. So instead of using the OwDeviceParameterMap class and create a new instance of it, you create a new class and a new instance of that anonymous class for every variable (there should be class files with a '$' for every instance in your jar).
I personally see no reason for new classes as it can be handled all using the OwDeviceParameterMap class (that only wraps a map).

[J-N-K]

I would leave it as it is now and keep it on my not-so-high priority list to refactor that for all devices in the future.

[maggu2810]

You could use: Collections.singleton(THING_TYPE_EDS_ENV)

[maggu2810]

Another option would be

Stream.of(OwSensorType.EDS0064, OwSensorType.EDS0065, OwSensorType.EDS0066, OwSensorType.EDS0067, OwSensorType.EDS0068).collect(Collectors.toSet());

but it does not harm.

But perhaps you should wrap it by Collections.unmodifiableSet, so the static final Set cannot be changed at runtime.

[J-N-K]

I changed that. What exactly is the difference between the two? Is Stream.of(..).collect() more efficient?