Re: [sigrok-devel] Protocol decoder and dumps for 1-Wire

[Iztok J. <izt...@gm...>]

Hi,

My replays are inline.

Regards,
Iztok Jeras

On Sun, Jul 22, 2012 at 3:08 AM, Uwe Hermann <uw...@he...> wrote:
> Hi,
>
> On Tue, Jul 17, 2012 at 10:15:41PM +0200, Iztok Jeras wrote:
>> First, I followed suggestions regarding rebase of git commits and
>> using the 'onewire' branch instead of 'master'. Could you check if I
>> did it correctly.
>> https://github.com/jeras/sigrok
>
> Yep, looks good, thanks! I merged the changes and fixed up the contents
> a bit wrt. some coding style / cosmetics and also some functional changes,
> see below.
>
>
>> I now separated the link/network/transport layers into separate
>> protocols, overdrive mode detection is now duplicated but not tested
>> yet. Each protocol is now placed inside its own directory, I would
>
> That's great.
>
>
>> prefer to place all *.py files into a single directory "onewire", but
>
> Nah, that doesn't make sense. The files belonging to one PD must be in
> that PD's directory, of course.
>
The main reason I would like to have more protocols in a single
directory is o more obviously share code, expecially for the transport
layer.
>
>> The transport layer code is very primitive for now.
>
> I renamed that PD to maxim_ds28ea00, as it doesn't really make sense to
> try to have a "generic" transport layer PD, I'm pretty sure about that now.
> This layer is inherently device-specific, so the respective devices will
> have their own PDs on top of onewire_link and onewire_network.
> I added some more explanations in the respective commit message.
>
> The PD looked like it's mostly intended for the DS28EA00 so I renamed
> and reworked it for that one. Please add other PDs for other devices as
> needed.
>
> Note: It _might_ make sense to have one PD support multiple devices, if
> those devices are _very_ similar. We did this for (e.g.) the lm75
> decoder, which can easily support the original National LM75 temperature
> sensor, but also various compatible devices from other vendors, which
> are usually almost identical, with only minor differences like 9-12 bits
> resolution as opposed to fixed 9bits for the LM75.
>
> It might make sense here to have a PD which handles the DS18B20 and
> various very similar sensors (similar features, commands, protocol, etc).
> Be careful though not to merge in too many devices. If there are too
> many differences between them, the code gets really ugly and convoluted
> and it would be better to have multiple PDs instead. Only support _very_
> similar devices within one PD.
>
It might make sense to have a single PD for all thermometers. One of
the reasons I created a single transport layer PD is that many
different 1-wire devices can be connected to the bus at the same time,
but it is not possible to connect multiple decoders to the network
layer PD. Another issue is that the user might not know what devices
exactly are connected to the wire, and a common PD could do it
automatically. If the issue is having the code organized, I would
split the code into various files in the same directory, and import
them all in __init__.py. I am still arguing for my position, since I
am not sure we already reached a good solution.
>
>> I also updated the reported timing, it is now start/end time, before
>> this was start/duration. This was due to an error in the
>> documentation:
>> http://sigrok.org/wiki/Protocol_decoder_API
>> Functions put() and decode() list the time/duration pair instead of
>> ss/es (start/end sample).
>
> Yeah, that page is a bit outdated, thanks! Will fix it soonish.
>
>
>> For the network and transport layer I would need to perform 8 and 16
>> bit CRC. I would like to use the 'crcmod' Python library
>> (http://pypi.python.org/pypi/crcmod/1.7), but it is not yet in Debian
>> repositories. What would be your suggestions here.
>
> Generally it would be best to only use common modules that are part of
> the Python 3.x distribution, as anything else may or may not be
> available on all systems (at all, or without extra work for the user).
>
> Think Windows, FreeBSD, OpenBSD, Solaris, embedded stuff with only a
> minimal set of installed packages (OpenWRT and similar, for example),
> and of course the 100+ Linux distros which may or may not carry
> non-standard Python modules.
>
> In this specific case, the CRCs don't look that hard to calculate,
> it should be doable with a small helper function in the PD's .py file,
> no need for an extra module, IMHO.
>
OK, the CRC function is very specific and it should not take more than
a few lines of code.
>
>> Another issue I see is with the extra long CLI needed for decoding:
>> sigrok-cli -i ../../../onewire.sr -a
>> onewire_link:owr=0,onewire_network,onewire_transport -s
>> onewire_link,onewire_network,onewire_transport -A onewire_transport
>> Is it possible to shorten this, or is the idea that for ease of use
>> the GUI will be used instead of the CLI?
>
> Well, yeah, this kind of stuff will always be a bit longer on the CLI,
> but that's not a big issue. The more convenient usage for users will be
> in the GUIs later, sure.
>
>
> FYI, I added a few additional dumps of a DS1985 iButton to the
> sigrok-dumps repo, so we have a few more files to test the PDs on. I'll
> write a specific PD for that device soonish, but _link and _network
> seem to work OK on it already.
>
> I may have a few more upcoming changes for the _link/_network PDs,
> haven't looked at all details yet.
>
I was able to create some overdrive waveforms this week, they are on
my GitHub sigrok-dump repo, but not yet on an appropriate branch
(master for now), and the README file needs some more details. Instead
of checking if overdrive is working properly I found big issues with
the link layer, I am working on a rewrite now. The separation of
layers, sounds as a very good idea now.
>
> HTH, Uwe.
> --
> http://hermann-uwe.de     | http://sigrok.org
> http://randomprojects.org | http://unmaintained-free-software.org