Sporadic lock fix and massive refactoring

[sabitov-kirill]

This pull request aims to fix old issue (#7). To achieve this I added small time gap between request during device initialization (8b395d3). This removes locks completely, I was able to run (previously it crashed on 3-4 iteration each time):

$ for i in (seq 1 100); echo $i; sudo ./fnirsi_logger.py &; sleep 3; kill $last_pid; end
$ for i in (seq 1 100); echo $i; sudo ./fnirsi_logger.py &; sleep 7; kill $last_pid; end
$ for i in (seq 1 100); echo $i; sudo ./fnirsi_logger.py &; sleep 13; kill $last_pid; end

Tested on Fnirsi FNB58, firmware v0.63/v0.68.

Refactoring

Additional, second commit includes total refactoring:

• removed frequent

  if args.verbose:
      print(...)

  blocks, using logging library
• used int.from_bytes instead of manual numbers conversion from little-endian bytes
• configuration and device specific options wrapped to enums and dataclasses for type safety and further extensibility
• functionality encapsulated to separate class, allowing using your project not only as separate executable, but as library

I understand that second commit completely changes the structure of the code and you might want to not merge it to main. It's all up to you, I just wanted to share what helped me, to solve my problems

[firepond]

It works well on my FNB58 v0.68! Thank you!

[baryluk]

Hi @sabitov-kirill

That is really nice work.

The fact that you found a way to fix the sporadic lock is amazing!

In general I like most of the changes. Thank you.

There was a reason, why the original code had everything in the main, as it makes things linear, and easy to hack, and to understand. But I think we can probably live with one extra class and some methods, and most of the code is now ready to refactor and cleanup indeed. I can even see, it being easier to use as a library, or when controlling multiple meters from a single python process (I usually just start multiple separate processes on separate devices, to do multiple measurements). ;)

But I have few small requests before the merge.

• restore use of str2bool in argparsing. Otherwise --crc=false parses incorrectly, as args.crc == True
• please use print to sys.stderr maybe?. No timestamps, or log levels. Just one verbosity level. Python logging library is great, but not really in this use case. Maybe changing to format='%(message)s' is also acceptable here.
• Please move _setup_crc out of the Meter class, to be a global function instead.
• Please move bulk of find_device method, out of Meter class, and make it a global function, that returns a tuple, or None, if it cannot find device. (The reason is that find device not only finds a device, but also modified the state). Then in Meter.find_device(self) , do: self.device, self.device_info = _find_device(). In general, number of methods on a class should be minimized (and methods that do not touch class state, should not be in the class at all, not even as static methods), because then reasoning about the class state is harder, and moving more logic out of class, makes it clear that they do not modify any class state, just return things via values.
• Reformat with black. I see some stray spaces, here and there and not quite following PEP8. (The exist code probably does not follow it either, but we probably can do it now).

Also please update a README, with new Python version required (due to dataclasses requirement).