Thank you very much for the constructive criticism. That is the first
time that I contribute to such a project.
So far I think I fixed all the issues you mentioned. The repositories
can be found at:
https://github.com/BenediktO/libsigrokdecode.git
https://github.com/BenediktO/sigrok-test.git
https://github.com/BenediktO/sigrok-dumps.git
I hope that the decoder now meets the conventions.
Regards
Benedikt
On 12.10.19 18:08, Gerhard Sittig wrote:
On Fri, 2019-10-11 at 21:59 +0200, Benedikt Otto wrote:
I'm Benedikt and I developed a decoder for a 7 segment display.
It supports common anode and cathode displays, the dot point and decodes
digits 0-9, a-f.
That's a nice first implementation. Though I can see a few issues
with it, which are easy to address.
This is the first time that a decoder instance name starts with a
digit. Can't tell whether that's OK (not strictly in Python, but
in the remaining environment of the application, including
external applications). My gut would make me find a name that
suits as a programming language identifier. Since it's a first,
see if you can rename the file to something else, just to be
safe.
Style nit: There is excess whitespace. Multiple levels of
indentation and multiple empty lines where one would suffice. As
much as I like TABs (and use them in private Python scripts), the
project decided to use TAB in C language sources, but use four
SPACE characters in Python. Also make sure to close braces where
they get opened, avoid "hanging text".
May be useful to also detect '-' (minus) in addition to blanks
and hex digits. More alternative forms for digits/letters which
can be presented differently are easy to add later ('7', 'c',
etc).
Style nit: Also add the comma after the last element in a list or
tuple assignment. Makes it easier to add more items, reduces
diffs later.
Please do name the inputs a-g (and dp) as these are the segment
names people are accustomed to. The "d0" et al names feel strange
and less accessible (unfamiliar). Maybe change "Signal A" to
"Segment A" and "Signal DP" to "decimal point" in the channels()
text labels?
I'd question the appropriateness of continued "cannot decode"
annotations when the input is none of the patterns known to you.
There's the question whether that's an error condition and worth
shouting, or whether it can pass silently. The gaps in the output
will be as apparent to users as the error annotations would be.
Considering the already limited scope of the decoder (only detect
[hex] digits), not being able to recognize the segment pattern
seems to be non-fatal by design.
Given the purpose of the decoder, I'd also question whether the
"lengthy" "Digit: ..." text labels are too useful. They kind of
shadow or bury the details which users actually wanted to see.
You can make use of the v3 API, the current implementation looks
like v2 and is more expensive as well as a little more complex
than it needs to be. Have the .wait() method return when any of
the input signals had any edge ('e'), instead of constantly
checking for changes in the .decode() method.
See if you can prepend the commit message's caption with the
decoder's name. This supports those who check the history and
like to navigate faster. See the VCS history for examples.
I added 3 tests, 2 of them are artificial, the last one is captured with
a Saleae 24MHz 8 channel clone while an Arduino controlled a display so
that i could verify that it displays the correct digits.
It's nice to see that you provide sample captures as well as test
cases with your decoder implementation. Can you also provide a
little README with the capture files? So that users know what to
expect before opening them and have a guess. :) Also assign the
a-g names to the traces before saving. Lets the decoder
auto-assign decoder inputs to traces, and serves documentation
purposes.
I hope that i generated the patches correctly, i was not able to create
one for the *.sr files, because they are binary files. Everything is
contained in the attached archive.
The 'git format-patch' command should handle binary files
perfectly fine, did that in the past. Haven't checked whether the
--binary option is required though, or active by default. But
it's available when needed.
If you plan to submit more changes, or expect several iterations
of feedback and improved versions, it's easier to provide the
link to a public repo here than attaching a set of files. Lets
reviewers and maintainers grab the files the way they are used
to, reduces traffic upon updates, results in less clutter in the
inbox and on disk, etc etc. :)
Thank you for working on that decoder!
virtually yours
Gerhard Sittig
--
If you don't understand or are scared by any of the above
ask your parents or an adult to help you.
_______________________________________________
sigrok-devel mailing list
sigrok-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sigrok-devel
_______________________________________________
sigrok-devel mailing list
sigrok-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/sigrok-devel
