1-wire controller and thermometer binding

[maloo]

Implementation of proposed API in #197.

Code sample to scan for devices, enumerate buses and devices and print temperature of all thermometer family devices found.

[maloo]

Should fix #172

[krwq]

Generally this looks mostly good to me, the bus enumeration and 64 are two things we need to fix up.

[maloo]

I could set the default to -1 instead of 64 and interpret that as don't change it from default.

[maloo]

As for bus I didn't see any response to my comments/questions. Maybe you could elaborate about what it is exactly that you don't like. Is it the use of an object for bus (which I need to do operations on like scan/search etc)? Or is it the fact that I allocate bus objects in OneWireDevice.EnumerateDevices() (which could be optimized to not using EnumerateBuses()+bus.EnumerateDevices())?

[krwq]

@maloo I've already responded above but seems something bugged there and doesn't display correctly.

For 1st question:

• -1 sounds good but we cannot use value 64 without any non-GPL licensed sources (i.e. if you find it in the public docs then it's fine) and we should also remove a link to GPL2 code. Arbitrary constant is fine (except 64) but please document it is arbitrary so that we don't need to scratch our head about it in 5 years in case we need to change it 😄 Biggest concern about this Scan API is that I'm not sure how Windows will or is implementing it and I feel like it's leaking implementation detail a bit. I'd rather have us call it automatically before enumerating even if it's heavy and slow. User can store the output if they find it too slow and for whatever reason they need to enumerate it more than once.

For 2nd question:

• enumerating buses: I don't like allocating object which might potentially grow in the future (i.e. because of Windows implementation). The way I see it you enumerate bus names as strings and then construct bus from the string and store it in the field. This method I think should be static,
• enumerating devices: similar comment as above except it should live as an instance method of the bus (note it can also enumerate strings if it is an instance method)

[maloo]

I can't find a nice solution with bus as id during enumeration. I keep having to new up the same amount or more OneWireBus instances. I see two use cases (see 1-wire sample).

1. Simple, I just want a list of e.g. temp devices. So I do OneWireDevice.EnumerateDevices(). This will have to return the device instances, because user obviously want to interact with the devices when using this API. Then there are two options for OneWireDevice. It could have the property Bus or BusId. If bus, enumerating the devices means I have to create the bus instances. If BusId, the user would have to new up a bus instance every time it want to do a bus operation like start a scan for device changes. In the second case you might get away with not allocating the bus object, but you loose the control of how/when the bus objects are created. If Windows would come up with a solution where a 1-wire bus instance is expensive to create, I would like to hold on to it for as long as possible just like you said in previous comments when discussing file handles. Best way to do that is to have 1-wire bus control when to create the instances and how to cache them. I would even want the Controller we removed, to allow implementation control over the "session" so it could cahce any expensive objects. Removing the controller forces EnumerateX() to create new instances or have a static cache that is never cleaned (like it would be in OneWireController.Dispose()).
2. Advanced usage, enum buses and then devices. Here user will obviously need the bus device, otherwise he would use the simple device enumeration.

[maloo]

Or, we make bus/device structs OWBusId/OWDeviceId w/ same operations as OWBus/OWDevice. That is technically the same as string id and static operations taking id param. But then I would like to get back OWController w/ Dispose to control caching and state lifetime for future platforms with heavyweight device state.
And could make ReadTemp an extension method on devid and skip family type instances to make common use case easy like now.

[krwq]

The code suggestions are what I meant

[maloo]

Yes, that far I got it :)
I might be slow, but I don't understand why? Just to get me on board with the change ...

1. Which use case benefit from this change in the public API?
   a) The basic OneWireDevice.EnumerateDevices()
   b) More advanced OneWireBus.EnumerateBuses() w/ bus.ScanForDeviceChanges/EnumerateDevice()?
   c) Other? ...
2. What exactly is the problem with the current one?
   a) Is it the number of allocations of OneWireBus? Which I believe will be the same in a real world use case.
   b) Is it that an eventual Windows OneWireBus object could be heavy weight? Which I believe should be cached and have the dev/bus instances to be references instead.
   c) Is it the OneWireDevice.EnumerateDevices() that allocate and throw away OneWireBus instances during enumeration? That is internal and could easily be optimized, but for no real gain in the Linux version.
   d) Other? ...

I believe I have listed the pros/cons with the string only solution above, and it in not the API change itself, but rather the changes it force in the other part of the API that I don't like. And the use cases listed in the sample code that get messier.

[krwq]

@maloo do bus id / device id change when rebooting? If they don't then you can avoid enumerating and allocating all together if user stores ID - if it's not possible on linux it might be possible on different OSes we might support in the future.

Other reason is that it follow patterns we do in other places:

• enumerating files enumerates names and does not create FileStreams
• SerialPort.GetPortNames also enumerates strings and not SerialPort instances

also enumerating string allows users do display something sensible in the UI

[maloo]

Ok, so the main reason is SerialPort/File enumeration style.

IDs don't change, but devices come and go dynamically so background scans or user requested bus scans can change What devices are enumerated.

My main objection to the id only solution is the user's code:

foreach (var dev in OneWireDevice.EnumerateDevices(DeviceFamily.Temp).Cast<OneWireTempDevice>()) {
    var temp = await dev.ReadTemperatureAsync();
    ReportTemp(dev, temp);
}

would now be:

foreach (var busId in OneWireBus.EnumerateBusNames())
{
    var bus = new OneWireBus(busId);
    foreach (var devId in bus.EnumerateDeviceNames(DeviceFamily.Temp))
    {
        var dev = (OneWireTempDevice)OneWireDevice.CreateDevice(bus.BusId, devId);
        var temp = await dev.ReadTemperatureAsync();
        ReportTemp(dev, temp);
    }
}

The second one is just a bit too complex for me just to keep style with File/SerialPort ;) It is also forcing user to instantiate a OneWireBus, in the other it is not needed (internal details that can be fixed if needed).
Maybe you have another picture of how to do a temperature sampling of available sensors using ids in the API?

Common ground? What about supporting both? EnumerateBusNames/EnumerateDeviceNames() and EnumerateDevices()

[maloo]

Looking back in the comments, maybe we have common ground. Do you want the id solution for bus only or bus+device? I first interpreted your comments to want both, but in later comments I see you only mention the bus. So OneWireDevice.EnumerateDevices() would be fine?

[krwq]

@maloo IMO EnumerateDevices (assuming it's an instance method on OneWireBus) should enumerate only ids (strings) since bus id is implied by the instance.

For the concern about casting, if you enumerate strings then you could potentially do:
bus.EnumerateDeviceIds(DeviceFamily.Thermometer).Select((id) => new OneWireThermometerDevice(id)) which doesn't seem bad

[krwq]

LGTM after fixing comments

[krwq]

Thank you @maloo! I'm still slightly skeptical about the IsCompatible (perhaps the name) but we can re-iterate on that if we get some issues about it