-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mixed set of changes #21
base: master
Are you sure you want to change the base?
Conversation
I've fixed up test suite, so travis should turn green. |
We need to report both "OFF" and "ON" states separately, so a single command line argument does not cut it. Eliminate the command line argument and simply use "ON" and "OFF" strings. Signed-off-by: Balazs Scheidler <[email protected]>
Signed-off-by: Balazs Scheidler <[email protected]>
UniPi supports both digital and relay based outputs. The first being low-current, low-voltage outputs (e.g. transistor based), the latter being high current, high voltage (e.g. relay based). The two behave almost identically, so digital_output can cover both with a few changes: the files in sysfs start with "ro_" instead of "do_", this patch adds support for relays in addition to digital outputs. Signed-off-by: Balazs Scheidler <[email protected]>
This seems to be a leftover, we don't need to listen to both the mapped and the unmapped topics. Signed-off-by: Balazs Scheidler <[email protected]>
Signed-off-by: Balazs Scheidler <[email protected]>
Signed-off-by: Balazs Scheidler <[email protected]>
Signed-off-by: Balazs Scheidler <[email protected]>
Previously, unipitt.Handler got the values of command line arguments as parameters as well as was responsible for reading the configuration file. This made it a bit difficult to add new and new parameters (such as MQTT authentication data, or as planned a prefix for topic names). To remediate this, this patch gets rid of the separate parameters to NewHandler() and relies on a Configuration instance to carry arguments to the constructor. This allocates reponsibility a bit better, NewHandler does not need to care what is in the configuration and what was passed as command line arguments. This simply becomes a user interface issue. Signed-off-by: Balazs Scheidler <[email protected]>
Signed-off-by: Balazs Scheidler <[email protected]>
Hey, thx for your input, will have a look at it later today! Sorry I only notice it now, but I haven't worked on this project for quite some time (+ I was also off for the Christmas holidays). I have been wondering though to change this setup a bit over the holidays. Currently, I actually use an integration into evok websockets: https://github.com/mhemeryck/evok2mqtt. I would like to align this project more with that one, i.e. in the use of the MQTT topics. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your input, I never really considered someone would take an interest in this code, never mind submitting a PR 👍
My main remark would be in the cmd/main.go
entry point that you now need to use multiple imports from the untpitt
package, where before it was by design just using the Handler
type. It's actually related to why the unit tests are now breaking. Would you be OK with just passing an optional configuration file to the handler instead, and have the handler deal with that (from within the unipitt
package?
Style: I do try to strictly follow the go fmt
style; just run go fmt
against it. Also check the documentation strings, some are outdated.
unipitt.go
Outdated
// Determine topic from config | ||
log.Printf("Trigger for name %s, using topic %s\n", d.Name, h.config.Topic(d.Name)) | ||
switch (d.Value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal preference for if/else
-- but OK like this.
btw, for my other evok2mqtt
project, I have the "trueish" and false-ish
payload configurable via a CLI flag (with defaults of ON
and OFF
, like you have). A use case could be that you could switch the behavior based on whether you use the contact as an NO (normally open) or NC (normally closed) contact. In practice though, I've never even bothered to overwrite these 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all started as a throw-away PoC whether I can make this work. Also, this is my very first go program I touched. I had a hard time compiling it in the first place :)
The "TRIGGER" string we had earlier wasn't working for my use-case, as I am implementing push buttons in my house and I want an action when I release the button.
I don't mind making this configurable, but I didn't want to add it to command line options, but with the latest configuration changes this would be trivial (but as far as I remember, you are challenging that in one of your upcoming comments).
I have changed the switch to if-else, looks much better indeed. Pushed as 815985d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am leaving this conversation open for the configurability part.
cmd/main.go
Outdated
// Initialize config from command line | ||
config := Configuration{ | ||
SysFsRoot: sysFsRoot, | ||
Mqtt: MqttConfig{ | ||
Broker: broker, | ||
ClientID: clientID, | ||
CAFile: caFile, | ||
Username: username, | ||
Password: password, | ||
}, | ||
} | ||
|
||
// Check if there's a config to be read | ||
if configFile != "" { | ||
log.Printf("Reading configuration file %s\n", configFile) | ||
err := updateConfigFromFile(configFile, &config) | ||
if err != nil { | ||
log.Fatal(err) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work right now, since Configuration
, MqttConfig
and updateConfigFromFile
are only known within the package unipitt
(cmd/main.go
is in the main
package). It's actually the reason the unit tests are broken.
You could just replace Configuration
and MqttConfig
by unipitt.Configuration
and unipitt.MqttConfig
; updateConfigFromFile
would need to be exported (upper case name, i.e. UpdateConfigFromFile
). I'm not sure I really like that thought, the whole idea of this Handler
type was that it would be the single entry point into the unipitt
package, now it's sort of split. You could argue to have something like a configuration handler apart from the main Handler
, but I wouldn't use more imports than that in cmd/main.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also: preference to use MQTTConfig
instead of MqttConfig
(I think it's even a recommended go best practice).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have renamed Mqtt to MQTT now in 9652400
The unit tests were fixed locally, which I have pushed 10 minutes ago and I did exactly what you described: I exported the functions and added qualifications to the types. I am a go newbie after all.
In any case, I argue that the configuration and the command line should somehow get merged before the thing gets to unipitt.Handler, which shouldn't care if an option was populated from the command line or a config file or any other mechanism. I've probably open coded a bit too much in main.go, but I would delegate the processing of the command line and reading of the configuration file where they belong: the main program. Then provide a couple of helper functions for this, so that most of this would be a one-liner in the main program.
Alas, the reading of the configuration file now overrides anything passed from the config file which is not the way it is usually implemented (rather programs initially read the config and command line options override those values). So this could need some improvements.
btw, I extended the config file because I needed yet another option, topic_prefix, which is a string that all mqtt topic names are prefixed with.
Let me know what you would like to do here, for now the unit tests should pass.
For the rest; this is unrelated to your PR, but I have recently been giving this whole project (and also the overall unipi setup) a lot of thought lately. It's been a while since I have actively worked on this project. For my current unipi setup, I use (my own) python implementation called Basically, it's a python implementation that bridges between the evok websocket implementation and MQTT. Longer term, I did want to have a way to directly hook into changes as they come in from the IO boards; hence my fork of unipi-tools, see https://github.com/mhemeryck/unipi-tools/tree/mqtt Even though
However, I've recently learned unipi no longer supports this SPI implementation and given the issues I did face with getting everything (cross) compiled in C, I figured to ditch that approach -- hence why I was rethinking just using this unipitt project. After my experience with
Additionally, I would also like to add something to be able to track how long a given input button is pushed, such that I can use that as an input for light dimmers (although, the longer I think about that, this is probably a project on its own). |
As requested in a github comment. Also cover this case in tests. Signed-off-by: Balazs Scheidler <[email protected]>
Signed-off-by: Balazs Scheidler <[email protected]>
To make the configuration file use underscory tags instead of the automatically generated ones. This will match them up with command line options too. Signed-off-by: Balazs Scheidler <[email protected]>
I had some uncommitted local changes, which I have just pushed. I am addressing your comments in a moment. |
Signed-off-by: Balazs Scheidler <[email protected]>
Signed-off-by: Balazs Scheidler <[email protected]>
Not at all. It took me a while to converge to unipitt and I like discussing things with like-minded individuals. See me further comments below.
evok is slow. It's latency on reporting events is OKish, but its CPU use is pretty high, continuously using something like 20-30% on my unipi. I know the poll frequency can be decreased, but I don't want to lose events. It's faster than homeassistant doing modbus, but still. I spent a considerable amount of time studying the kernel code behind the /sys interface unipitt is using and even though the kernel is polling the boards too (e.g. not interrupt driven), it is still the best approach that we can use:
This means that it basically guarantees that the value we read is not older 20msec (configurable). If userspace is not reading these values, the kernel will not poll in the background. If userspace reads faster than once 20 msec, the kernel will return a cached value.
All in all, not yet interrupt driven (which I was looking for), but as close as it gets. IIRC evok will use the modbus underneath, so quite a fat stack just to query the value of an input pin.
exactly. the kernel (at least the time I was reading its code) uses SPI to directly communicate with the I/O boards, so modbus is bypassed entirely.
As I see mqtt does not support timestamps at the protocol level and I am not sure homeassistant supports timestamps within payloads. But sure, in principle the timestamp that we could add in
Yes, I was about doing exactly that. The state of the output needs to be tracked too.
I have just added a topic_prefix option, but that's basically your client_id. Although client_id might be the name of a physical device (I used the hostname of my unipi), whereas in MQTT topic naming I would use a logical name. (e.g. I named my unipis unipi1 and unipi2, the first one controls covers, the second one covers lighting and everything else). I don't know if overriding the topic names is that important. I would map the internal names to friendlier names in homeassistant.
probably, at least the state topic.
env variables are not that much better, see
I have no opinion on go dep vs go mod, I don't know either :) I really like github actions though, we use it heavily in another open source project I participate in.
I was doing these kind of things on top of homeassistant with appdaemon. The timestamp I get there is not as accurate, but since these are human interactions that is not that important either. And in homeassistant (and thus appdaemon), you can trigger on events like: "if this button is pressed for 0.5 seconds, then do this or that":
|
Signed-off-by: Balazs Scheidler <[email protected]>
Signed-off-by: Balazs Scheidler <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more small remarks. Looks OK for the rest to me.
Haven't actually tested it myself yet, will try to give it go tomorrow or this weekend.
@@ -57,6 +57,30 @@ func TestConfigurationTopic(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestConfigurationTopicWithPrefix(t *testing.T) { | |||
cases := []struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be sure: this will still work without the prefix as well, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, without the prefix option it works just the way it was working before, there's a testcase before this one that checks it.
But if you want, we can get rid of topic_prefix in favour of a more structured topic structure that comes by default, that will probably make it easier to introduce mqtt discovery in the long run. So do let me know if you want me to transform topic_prefix into a new topic formatting scheme that you suggested in one of your previous comments.
Let me know if you want me to split the series into smaller PRs so that it becomes easier to merge. I think there are a few patches that are trivial and could go in immediately whereas there's a small number that would take more time to mature.
Yes; tbh, the main reason I used evok was that I wasn't that convinced of unitpitt at the time and I quickly needed to hack up something together, which I could easily do in python. First version of unipitt probably took me about 2 weeks, evok2mqtt was written in an afternoon.
I started with the other unipi-tools repo. I was actually able to poll the boards with the utility functions they provided directly over SPI. However, at the point that I needed to cross-compile an MQTT library, I got stuck. It's interesting to hear you pretty much thought of all the same things I did 🙂
Indeed, interrupt driven was also the thing I was looking for. I still think it's a bit of a pity knowing that there's kernel code that can do the polling and write this information into the sysfs tree -- that means it should be possible in theory to push out MQTT events as well. btw, another option I did consider was Another library that does use this sysfs / epoll way of working is periph.io https://periph.io/ https://pkg.go.dev/periph.io/x/host/v3/sysfs#SPI , IIRC they also used epollctl on some sysfs files to get edge triggered updated on GPIO pins -- but given I couldn't get epoll working on the sysfs files on my unipi, I'm not that optimistic this could would work for unipi.
I didn't even think of the fact that you don't have the timestamps from the MQTT events 🤔 I probably wouldn't change the basic payload for the state topic, but instead track a push button state as a separate FSM, really intended to express some incrementing or decrementing value (for dimmer switches or covers, shades).
Sounds very similar to the way I named the topics in evok2mqtt. if no client_id / topic prefix is given, the hostname is taken as default.
I'm not that familiar with appdaemon, actually; how's it different from home assistant? I am running a dockerized home assistant in a kubernetes (k3s) (single node) cluster. All of the topic configs, links, ... are in yaml and deploys are repeatable and autohealing this way. Anyways, nice to be able to discuss with someone else on this very niche issue 🙂 Sorry I'm not always able to respond right away the past few days (I'm actually working right now on a LED strip setup with the axon DALI driver unipi offered last year -- I don't see it on offer anymore). |
Signed-off-by: Balazs Scheidler <[email protected]>
I think the intricacies of the SPI interface is best encapsulated in the kernel, so no need to run that in user-space. And unipi nicely provides us with the sysfs interface, which is much nicer than having to communicate using SPI messages and doing bit banging to get stuff right. So, carefully evaluating all options (SPI, unipi sysfs, kernel gpio interface, modbus, evok) I concluded the best course of action was simply to rely on sysfs, which is exactly just what you did in the first place, that's why I started using unipitt instead of writing it from scratch.
the unipi kernel driver does not implement polling, thus epoll/poll/select will not work on these files. To do epoll() the kernel driver would need to implement the "poll" method in its file_operations, which the unipi driver doesn't. And the only way it could implement poll, if it was running a timer or a kernel thread internally that would poll the I/O board via SPI and report that via this poll method. I think it wouldn't improve the current situation too much, where this looping is in the userspace application. This means that the The limit on the granularity of this polling depends on these settings:
The first can easily be tuned and is 50Hz by default (e.g. 20msec), the second allows roughly 12Mbit/sec or 1.5MByte/sec. Each SPI message is roughly 6-8 bytes if I remember correctly, so in theory the SPI bus would allow 150k polls per second, but I can imagine that the ARM core in the slave wouldn't be able to cope. With all that said, I think the sysfs based interface can be scaled up to 1msec resolution, but 20msec was good enough for my use-cases, I am using manual push buttons and I couldn't trigger a lost key-press, even though I was trying. unipitt takes up 5% of my CPU while the kernel uses also 5% system time, so that's roughly 10% for polling 3 I/O boards. I am not using these Neurons for anything else, so I am ok with that. This could be improved in the kernel side. If I remember correctly, the slaves would allow reading blocks of pins, whereas the current kernel driver only reads 16 bits at a time, so if we are doing bulk reads, we could immediately read all of them in one go, but I am not sure this would speed things that much, maybe the system time would get lower.
Yup, that could be useful. Homeassistant is not necessarily best suited for low-level logic, so it might make sense to implement low-level logic into unipitt and then work with higher-level events in homeassistant, e.g. instead of doing the time-tracking in HASS, trigger a "Long press" event in unipitt, that can trivially be used in HASS.
Oh, yes. The deployment side of things are not very shiny in my case :) I have a growing number of devices and the code running on each is - well - manually maintained :) Too many times I have run into stuff where something was different from one node to the next.
Yeah, too many projects with too little time. unipi is only used in our holiday home, so I tend to work on these kind of things when I am there. In our main house, we have a completely different setup. With all this said, I think unipitt could be an important building block of making unipi a better player in the homeassistant ecosystem, especially if we were to support MQTT auto-discovery. And considering how many people want relays/inputs in HASS, which they hack together via RPi and HW-316, I think this project actually make sense and is not a "niche" problem. People simply don't consider UniPi for these use-cases, simply because HomeAssistant's modbus support is not really suitable, evok integration does not exist and neither does MQTT. (well except for unipitt, which is not - yet - perfect). |
btw, there's some kind of timing issue in the test suite that causes every 2nd/3rd run to fail, that's what happened in travis. |
I don't think it's solely a timing issue; goreleaser also reports some issues. That's probably because the Here's a patch that would fix it (I only realized after I did commit that I can't push it)
You can then check a local snapshot build using
For more details, check goreleaser |
Another remark; as for the race condition issue: these are often tricky to debug (certainly since I'm a bit rusty on this code base ...). I did notice a number of "brittle" tests in Long-term, I should probably find another solution to this issue. |
This branch contains a number of independent patches I use locally in my setup, that I would consider pushing upstream. I can't see too much changes since its original release, but nevertheless the code was useful to me as it was.
Any feedback is appreciated.