search

nerves-networking / vintage_net_wifi

WiFi networking for VintageNet
Apache License 2.0
32 stars 18 forks source link

Wifi event notifications #87

Closed dognotdog closed 2 years ago

dognotdog commented 3 years ago

This implements the suggestion in #86 naively, maybe there are ways to improve on this? I've tested it as far as that it does correctly emits CTRL-EVENT-ASSOC-REJECT, while CTRL-EVENT-SSID-TEMP-DISABLED seems a bit more elusive.

fhunleth commented 3 years ago

Thank you for this PR! I think this is a good way of implementing this and we should push it forward.

I just have a couple comments, and I'm thinking that you already know them and were just waiting for feedback on the general approach. Here they are anyway:

  1. Could you rebase?
  2. Instead of posting tuples. Could you make a VintageNetWiFi.Event struct with the fields? I have half-baked ideas for additional fields, but I don't want figuring them out to slow this PR down. Making them structs should allow for future expansion.
  3. Add a unit test.
dognotdog commented 3 years ago
  1. Instead of posting tuples. Could you make a VintageNetWiFi.Event struct with the fields? I have half-baked ideas for additional fields, but I don't want figuring them out to slow this PR down. Making them structs should allow for future expansion.

How should the non-applicable fields be treated? The two events described seem to have completely disjoint fields. Maybe creating unique types for each would work, in which case the "event name" would not have to passed on at all, and they could be matched on type?

fhunleth commented 3 years ago

I'm not sure how many event types there will end up being. How about just using a map that has one required field, :name and then has optional fields depending on whatever makes sense for now.

dognotdog commented 3 years ago

@fhunleth added the respective updates to use an actual Event struct and some basic unit tests.

dognotdog commented 3 years ago

Next time you send a PR, could you get CI to pass? It looks like this time it's just a few extra newlines that could be removed with mix format, but it's nice knowing that Dialyzer passes too.

Yea I didn't understand what was going on, doing mix format on my end didn't seem to do anything, but that might be due to some user config somewhere in the weeds... I'll dig into that some more

fhunleth commented 2 years ago

Merged in #129. Thank you for getting this feature started and sorry for the long wait.