baryluk / fnirsi-usb-power-data-logger

Driver / Data logger for FNIRSI FNB48, FNIRSI C1 and FNIRSI FNB58 USB Power meter
MIT License
141 stars 14 forks source link

Add option to set alpha, also exit if magic file name is present #20

Closed pdl-user closed 4 months ago

pdl-user commented 4 months ago

I've read your FAQ and strongly agree with your design principles and really appreciate the "keep it simple" approach and how quick and easy it was for me to get this up and running.

But for me alpha = 0.9 is too strong and any nonzero alpha spoils averaging. (I know temperature isn't critical, but still...)

Also, when feeding the output to a processing routine and saving stdout from that to a file, a Ctrl-C is caught by fnirsi_logger.py which cleanly shuts down but then it is passed to the filter which dies. This is particularly bad when the last stage of the pipeline is gzip.

I have forked your repository and added feature branches which implement an option to set alpha (and leave the default at 0.9) and to also exit if a file named fnirsi_stop exists in the current directory.

My fork is at: https://github.com/pdl-user/fnirsi-usb-power-data-logger

I'm not making a pull request yet, because I wanted to discuss it with you first. If you want to do discussions in a PR, I can make one.

Only set alpha: https://github.com/pdl-user/fnirsi-usb-power-data-logger/tree/set_alpha

Only stop reading: https://github.com/pdl-user/fnirsi-usb-power-data-logger/tree/stop_reading

Both enhancements: https://github.com/pdl-user/fnirsi-usb-power-data-logger/tree/both_alpha_and_stop

All the above are fast-forwardable

Also, are you open the enabling discussions for this repository?

baryluk commented 4 months ago

Hi @pdl-user

Yes, both of your proposed features are welcome and can be added. Feel free to make PRs.

Good observation about things like gzip. That is very much true, that doing hard Ctrl-C in bash will now abort the pipeline, and this could cause issues. It is not exactly due our script, but how the bash runs (by delivering signal to all processes in the process group, which means all processes in a pipeline).

Also, we can add a signal handler for SIGINT / SIGTERM, and exit cleanly, cleanup, and return exit status 0. Then you can send a SIGINT to the script itself, and it will exit cleanly. The advantage of this over using stop file (which is also fine), is that with stop file, you need later to clean this stop file to start the script again. And is problematic when you run multiple script instances (on multiple usb meters) in parallel.

But as I said, I am also fine with stop file. There is no harm adding it even with above problems mentioned.

Maybe just rename it to --temp_alpha. It would be nice to express it in more useful unit, like --temp_smoothing_half_life_seconds=0.5, but that requires knowing sample rate, doing some conversions with exponent function, and is a bit device specific, that is why I just went with a simple alpha factor, that looked right.

pdl-user commented 4 months ago

Thank you for your comments @baryluk . I guess I misunderstood something I read about Ctrl-C handling in python and whether it swallowed it or passed it on. I did not want to take on fancier hadling of Ctrl-C myself because of my lack of experience and also desire to avoid adding complexity.

I did give some thoughts in my early experimenting with the stop file to the issues that you raise. I originally had an option to set the full path and name and put the default in /dev/shm for speed and to save writing to storage. I haven't included that option in my fork yet in keeping with maximizing simplicity. (I actually do want to eventually have setting the path as an option.) I settled on current working directory since that allows simultaneous running on multiple meters by running in different directories (which itself might be inconvenient for some).

I considered automatically deleting the stop file from inside fnirsi_logger.py but that adds complexity and reduces flexibiliy. I envisioned having an alias, bash function, or bash script that could implement whatever cleanup policy the user wants.

I would like to keep the option name for alpha short, but if you want to leave open a possiblility to add filtering for other columns, I will rename it temp_alpha.

I will go ahead and make a PR for my both_alpha_and_stop branch and we can fine tune with later commits.

Thanks again for your consideration on this.