Hi Tim,
I’ve been working away form home the last 3 days and not managed to find time to comment (here or on github), I did start writing this comment before I left so it originally replies to your original post, with some additional replies to your subsequent posts.
Support for systemd would be a great addition, interested to see your PR for this, did you retain the use of the /etc/default/emonhub config file? and the ability to create the logfile folder if it doesn’t exist? - - Ok, so I see your PR now and the immediate thing that sticks aside from the complexity is the change to how emonhub logs errors, emonhub.log is the most useful debugging tool we have, therefore for backward compatibility, ease of use, clearer logfiles and to keep all existing how-to’s and previous forum threads valid, not to mention the emoncms config module and those users that read/parse the emonhub.log file into something else (not good practice IMO, but I know some users have done it), emonhub.log must remain, at least as the default solution. The 2 logfile approach works much better as in normal running all the actual systemctl messages will be lost in the sheer quantity of normal emonhub.logs and get rotated out, so you are effectively swapping the emonhub.log to the systemd emonhub log and doing away with any systemd records, unless you avoid using INFO or DEBUG loglevels. Plus emonhub can run on windows too, so the default also needs to cater for that. If the user chooses to do away with emonhub.log, that could be an option, but IMO not the default.
Very interested to see whats required for running under Python3, the original reason emonHub (or rather it’s predecessor OEM Gateway) was written in Python2 primarily because the ConfogObj module wasn’t, at that time ported to Python3, that’s no longer the case. I guess a version that runs on 2 and 3 would be preferable to ease the transition (for both users and devs with very little Python3 experience like myself) so I would be interested in knowing how much extra work that would entail.
I also have a working solution for restarting the crashed threads, although it hasn’t been tested to any degree as I do not have an “emon-pi” variant emonhub in service, it would be great to compare notes so to speak. — Now I read that your solution is in the systemd service, which IMO isn’t the best place to be doing it. The solution I was working towards, restarts threads from within emonhub without any need to restart. This avoids interrupting other threads that are working fine, the planned reintroduction of buffering will also be impacted by doing it in the systemd as any buffered data will be lost on a restart.
The clean-ups would be great and coding standards should be enforced as PRs are submitted, perhaps some automated Travis checks are needed as on the emoncms repo to assist contributors, correcting the coding periodically in this way messes with the commit history, you are right it needs doing, but it would be better to get it done prior to merging the PR’s IMO and that is in the hands of the repo maintainers @glyn.hudson and @TrystanLea.
I am pretty busy this weekend as I am away again Mon AM but I will try to review in more depth, I just thought I should voice my opinion sooner rather than later on a couple of points, hopefully before you invest much more time in them, Having said that I have no control over the oem/emonhub repo so the changes may just get merged regardless of my opinion.
If I get time I would also like to write a post about emonhub “thread is dead” issues, because the “cause” (incomplete or poor coding) needs to be addressed more importantly than the “effect” (the thread dying). In short 3 things need to happen.
- Better sanitation of incoming data, more errors need catching and handling within emonhub. It is the responsibility of each interfacer to ensure it handles the data correctly and filters away anything that could cause a problem.
- The threads need to report a trace back before dying, this would give us the info required to understand the crash and implement further checks and error handling.
- Restart the thread from within emonhub, it’s quicker, simpler and less destructive than restarting emonhub entirely.
This is the ONLY way that the issue can be addressed because regardless of how you restart the thread, emonhub or the device, without addressing the “cause” it will only happen again and again, for a range individual reasons, but the complaint will always be the same “my emonhub keeps restarting for no apparent reason”.
Although posted here on your thread @tim, these comments about (not) restarting emonhub are also in response to other suggestions along the same lines and hopefully to take the focus away from restarting and on to avoiding the crashes in the first place, which will never happen if we automate the restarting without getting tracebacks.