Add device extensions support #228

jjcarstens · 2024-09-01T13:10:31Z

Next idea of the previously defined "extensions"

This adds the mechanism to handle features on a device for custom data and/or reporting outside the update mechanism. Features are isolated from the socket as to not get in the way of potential updates and can be defined in external libs as well as this one by implementing the NervesHubLink.Extensions behavior.

lib/nerves_hub_link/application.ex

joshk · 2024-09-02T01:20:49Z

lib/nerves_hub_link/socket.ex

@@ -481,7 +506,7 @@ defmodule NervesHubLink.Socket do
      _ = Client.handle_error(reason)
    end

-    rejoin(socket, topic, socket.assigns.params)


why are the params removed?

Sorry, I meant to comment. The slipstream default is to reuse the params that were sent in the initial join. With the addition of features channel, this was sending the generic firmware metadata params in the event of the features channel closing and rejoining.

I could have pattern matched on the channel name to selectively choose, or just let slipstream send the default original params (which are already static anyway, so no need to forcefully send)

jjcarstens · 2024-09-02T18:44:00Z

lib/nerves_hub_link/features/features.ex

+    name = opts[:name] || raise "Missing required feature arg: name"
+    version = opts[:version] || raise "Missing required feature arg: version"


Changed to being arguments to the use macro rather than callbacks so that it can be enforced at compile time vs just a warning (not everyone has --warnings-as-errors flag by default)

These need some typing and docs that will come later as I'm out of time for a bit

jjcarstens · 2024-09-02T18:44:59Z

lib/nerves_hub_link/features/geo/geo.ex

Taken almost verbatim from https://github.com/nervescloud/nerves_hub_link_geo

joshk · 2024-09-03T00:22:06Z

Honestly, this code is looking solid to me!

lawik

Nothing to add. Should work well :)

Is there anything that needs to be rounded out or added?

lawik · 2024-09-03T12:28:13Z

lib/nerves_hub_link/features/geo/resolver.ex

@@ -0,0 +1,22 @@
+defmodule NervesHubLink.Features.Geo.Resolver do
+  @typedoc "Supported location sources"
+  @type sources() :: :gps | :geoip | :custom


Since users can add their own this should probably be a string :)
(was this lifted from geo, if so, kind of irrelevant to the PR)

Or {:custom, "where_i_put_it"} style. Give us something to display in UI about how it gets it's data.

lawik · 2024-10-28T16:06:41Z

Rebasing but should be good to review @joshk

This has had geo and health integrated into it.

I like the new abstraction. Nice and friendly.

lawik · 2024-11-12T15:39:24Z

Link won't load the Feature modules if run iex -S mix in dev mode. I'm fairly sure it would load more agressively in production compile on-device but this seems not ideal. Essentially a race condition between the socket going up and the library figuring out if it has features?

I tried changing it to not just run find_features/0 on init but rather on list/0 but that didn't really help.

lawik · 2024-11-12T15:44:56Z

I suppose since we plan on controlling this from nerves_hub_link we could provide a default discoverability for health and geo and then let people configure if they have more or other sets they want.

I'll move this to config.

lawik · 2024-11-12T16:00:58Z

Running this and nerves_hub_web locally, catching a bunch of trickiness :)

jjcarstens · 2024-11-12T16:16:43Z

It's because of how modules are loaded (i.e. :interactive vs :embedded). I noticed this last night as well:

iex(8)> NervesHubLink.Features.find_features
%{}

# This first call forces the module to load
iex(9)> NervesHubLink.Features.Geo.module_info :attributes
[
  vsn: [11907264823846348490321455620962448498],
  behaviour: [GenServer],
  behaviour: [NervesHubLink.Features]
]

iex(10)> NervesHubLink.Features.find_features
%{
  "geo" => %{
    module: NervesHubLink.Features.Geo,
    version: "0.0.1",
    attached?: false
  }
}

lawik · 2024-11-12T16:18:47Z

Yeah, I know. I switched it to config. Argue if you disagree :D

lawik · 2024-11-19T14:27:53Z

@jjcarstens @joshk please review this now, kindly, I think it is done

nshoes

This generally LGTM, just some small nits, nothing blocking.

README.md

nshoes · 2024-11-19T19:42:03Z

README.md

+
+## Configure Geo
+
+It is intended to be easy to replace the default Geo Resolver with your own. Maybe you have a GPS module or can resolve a reasonably precise location via LTE. Just change config:


Suggested change

It is intended to be easy to replace the default Geo Resolver with your own. Maybe you have a GPS module or can resolve a reasonably precise location via LTE. Just change config:

It is intended to be easy to replace the default Geo Resolver with your own. Maybe you have a GPS module or can resolve a reasonably precise location via LTE. Just change the config:

README.md

lib/nerves_hub_link/extensions/extensions.ex

lib/nerves_hub_link/extensions/health/report.ex

lib/nerves_hub_link/socket.ex

This adds the mechanism to handle extensions on a device for custom data and/or reporting outside the firmware update mechanism. Extensions are isolated from the socket as to not get in the way of potential firmware updates and can be defined in external libs as well as this one by implementing the `NervesHubLink.Extensions` behavior - Implements Geo and Health modules as examples of Extensions - All Extensions currently report only on request from NervesHub - Add optional vintage_net dependency to allow fetching interfaces

lawik · 2024-11-20T09:09:48Z

Good nit-hunt @nshoes, much appreciated :)

jjcarstens requested review from joshk, fhunleth, oestrich and lawik September 1, 2024 13:10

jjcarstens mentioned this pull request Sep 1, 2024

Add device extensions support nerves-hub/nerves_hub_web#1479

Merged

fhunleth reviewed Sep 1, 2024

View reviewed changes

lib/nerves_hub_link/application.ex Outdated Show resolved Hide resolved

joshk reviewed Sep 2, 2024

View reviewed changes

jjcarstens force-pushed the features-support branch from e91b8a9 to 1736c14 Compare September 2, 2024 18:40

jjcarstens commented Sep 2, 2024

View reviewed changes

lawik mentioned this pull request Sep 3, 2024

Extension pubsub #220

Closed

lawik reviewed Sep 3, 2024

View reviewed changes

lawik force-pushed the features-support branch from 9675d62 to d4f8a3a Compare October 28, 2024 16:02

lawik marked this pull request as ready for review October 28, 2024 16:02

lawik force-pushed the features-support branch from c3ec7c5 to 9ccb710 Compare October 28, 2024 20:10

jjcarstens force-pushed the features-support branch from 9ccb710 to dff5731 Compare November 12, 2024 02:19

lawik force-pushed the features-support branch 2 times, most recently from 5e2ca0b to b044770 Compare November 19, 2024 14:27

lawik requested review from joshk and removed request for oestrich November 19, 2024 14:28

lawik changed the title ~~Add device features support~~ Add device extensions support Nov 19, 2024

nshoes approved these changes Nov 19, 2024

View reviewed changes

lawik force-pushed the features-support branch from f08f78a to ff6b16a Compare November 20, 2024 09:09

lawik merged commit f0a9e84 into main Nov 20, 2024
5 checks passed

lawik deleted the features-support branch November 20, 2024 11:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add device extensions support #228

Add device extensions support #228

jjcarstens commented Sep 1, 2024 •

edited by nshoes

Loading

joshk Sep 2, 2024

jjcarstens Sep 2, 2024

jjcarstens Sep 2, 2024

jjcarstens Sep 2, 2024

joshk commented Sep 3, 2024

lawik left a comment

lawik Sep 3, 2024

lawik commented Oct 28, 2024

lawik commented Nov 12, 2024

lawik commented Nov 12, 2024

lawik commented Nov 12, 2024

jjcarstens commented Nov 12, 2024

lawik commented Nov 12, 2024

lawik commented Nov 19, 2024

nshoes left a comment

nshoes Nov 19, 2024

lawik commented Nov 20, 2024

		name = opts[:name] \|\| raise "Missing required feature arg: name"
		version = opts[:version] \|\| raise "Missing required feature arg: version"


		## Configure Geo

		It is intended to be easy to replace the default Geo Resolver with your own. Maybe you have a GPS module or can resolve a reasonably precise location via LTE. Just change config:

Add device extensions support #228

Add device extensions support #228

Conversation

jjcarstens commented Sep 1, 2024 • edited by nshoes Loading

joshk Sep 2, 2024

Choose a reason for hiding this comment

jjcarstens Sep 2, 2024

Choose a reason for hiding this comment

jjcarstens Sep 2, 2024

Choose a reason for hiding this comment

jjcarstens Sep 2, 2024

Choose a reason for hiding this comment

joshk commented Sep 3, 2024

lawik left a comment

Choose a reason for hiding this comment

lawik Sep 3, 2024

Choose a reason for hiding this comment

lawik commented Oct 28, 2024

lawik commented Nov 12, 2024

lawik commented Nov 12, 2024

lawik commented Nov 12, 2024

jjcarstens commented Nov 12, 2024

lawik commented Nov 12, 2024

lawik commented Nov 19, 2024

nshoes left a comment

Choose a reason for hiding this comment

nshoes Nov 19, 2024

Choose a reason for hiding this comment

lawik commented Nov 20, 2024

jjcarstens commented Sep 1, 2024 •

edited by nshoes

Loading