Add device extensions support #1479

jjcarstens · 2024-09-01T13:17:00Z

Start of device extensions which allow for specialized functionality on device to report data and interactions safely outside the update mechanism.

Requires nerves-hub/nerves_hub_link#228

Based on conversations the last while, these are some of the core design decisions so far:

NervesHub selectively tells a device what extensions to enable
Extensions are off by default on device
Extensions should have no affect on socket connectivity or ability to update a device

With that in mind, this is the basic communication cycle for supporting extensions on device.

sequenceDiagram
    participant Device as Device (NHL)
    participant NH as NervesHub

    Device ->> NH: Connect
    NH ->> NH: Check `extensions_allowed?`
    NH ->> Device: Send "extensions:get"
    Device ->> NH: Join "extensions" channel with [%{<extension> => <version>}]
    NH ->> NH: Determine extensions to enable based on report
    NH ->> Device: Send "extensions:enable" to with list of extensions to enable

    loop For each extension to enable
        Device ->> NH: Report "<extension_name>:<event>" where event is enabled or error
    end

TODO: All the bits to selectively decide what extensions can be enabled for a device

lawik

Looks like a good approach to me. Want me to flesh out the allow mechanisms as a branch off of this branch or something?

Or we merge this once we tidy up the dbg and TODO notes?

lib/nerves_hub_web/channels/device_channel.ex

lawik · 2024-10-28T16:07:58Z

Fixing the rebase. But ready for review @joshk and I've cut anything that would cause legacy for the yet-unreleased health and geo features. So two env vars have changed to match. Messaging internally has changed. Message to the link-side features have been aligned to all do similar stuff.

lawik · 2024-11-11T10:00:54Z

I need to manually test this locally to make sure everything works correctly. But would love review @joshk so when I've tried it properly we can move this and the matching nerves_hub_link along.

lawik · 2024-11-12T07:32:01Z

@jjcarstens was this just a rebase?

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

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

lawik · 2024-11-19T13:36:10Z

Marking as draft while I churn through renaming and so on.

Allows for specialized extensions on device to report data and interactions safely outside the without affecting the firmware update mechanism. - Geo and Health adapted to the new mechanism. - Product-level settings to enable/disable - Device-level settings to mostly disable

lawik · 2024-11-19T15:00:41Z

Re-wrote Jon's words above to be all about EXTENSIONS! 🤘🏻 👀 🤘🏻

I've renamed all the things. The sibling nerves_hub_link PR seems healthy and solid. I put one test on pending because it is just messy to update for now.

I think everything is good to go. Or at least ready to be picked over :D

lawik · 2024-11-19T15:01:03Z

So paging @joshk and @jjcarstens for reviewing :)

jjcarstens · 2024-11-19T16:09:31Z

lib/nerves_hub/devices.ex

@@ -1383,4 +1383,33 @@ defmodule NervesHub.Devices do
        "pending"
    end
  end
+
+  def enable_extension_setting(%Device{} = device, extension_string) do
+    device = Repo.get(Device, device.id)


Why are we fetching the device if we are requiring it as an arg?

jjcarstens · 2024-11-19T16:10:05Z

lib/nerves_hub/devices.ex

+
+    Device.changeset(device, %{"extensions" => %{extension_string => true}})
+    |> Repo.update()
+    |> tap(fn _ ->


I believe we will need to check that the update was successful before broadcasting, otherwise we will be out of sync with the setting

jjcarstens · 2024-11-19T16:11:03Z

lib/nerves_hub/devices.ex

+  def enable_extension_setting(%Device{} = device, extension_string) do
+    device = Repo.get(Device, device.id)
+
+    Device.changeset(device, %{"extensions" => %{extension_string => true}})


Does this actually work if I have multiple extensions? To me it looks like it will completely overwrite all extension settings with just this one?

It does not, embeds_one raises by default on replace. I think an on_replace: :update will work here.

jjcarstens · 2024-11-19T16:16:06Z

lib/nerves_hub/extensions.ex

+  @supported_extensions %{
+    health: """
+    Reporting of fundamental device metrics, metadata, alarms and more.
+    Also supports custom metrics. Alarms require an alarm handler to be set.
+    """,
+    geo: """
+    Reporting of GeoIP information or custom geo-location information sources
+    you've set up for your device.
+    """
+  }


Since this is a behavior module, I feel like we should keep as much of this in the extension as possible. Maybe we need a description callback for the module to define it?

jjcarstens · 2024-11-19T16:17:58Z

lib/nerves_hub_web/live/product/settings.ex

+      case result do
+        {:ok, _pf} ->
+          # reload extensions
+          assign(socket, :extensions, Extensions.list())


Extensions.list() is static. Do we need to reload it? In fact, we probably don't even need to hold it in the assigns and just render with the view. Save some memory

jjcarstens · 2024-11-19T16:21:03Z

lib/nerves_hub_web/channels/device_socket.ex

@@ -94,7 +95,7 @@ defmodule NervesHubWeb.DeviceSocket do
    |> Devices.get_device_certificate_by_x509()
    |> case do
      {:ok, %{device: %Device{} = device}} ->
-        socket_and_assigns(socket, device)
+        socket_and_assigns(socket, Devices.preload_product(device))


Do we need to include this preload in a more effect way? Like with getting the device certificate? I know Josh did a lot of work optimizing preloads so I'm hesitant to put such a small function in without thinking through

jjcarstens · 2024-11-19T16:21:27Z

lib/nerves_hub_web/live/devices/device_health.ex

@@ -34,6 +34,7 @@ defmodule NervesHubWeb.Live.Devices.DeviceHealth do

    if connected?(socket) do
      socket.endpoint.subscribe("device:#{device.identifier}:internal")
+      socket.endpoint.subscribe("device:#{device.identifier}:extensions")


What are we doing with this subscription here?

jjcarstens · 2024-11-19T16:35:30Z

lib/nerves_hub_web/channels/extensions_channel.ex

+      product_extensions
+      |> Map.from_struct()
+      |> Enum.filter(fn {extension, enabled?} ->
+        enabled? == true and Map.get(device_extensions, extension) != false
+      end)
+      |> Enum.map(&elem(&1, 0))


Suggested change

product_extensions

|> Map.from_struct()

|> Enum.filter(fn {extension, enabled?} ->

enabled? == true and Map.get(device_extensions, extension) != false

end)

|> Enum.map(&elem(&1, 0))

for {extension, true} <- Map.from_struct(product_extensions),

{^extension, device_enabled?} <- Map.from_struct(device_extensions,

device_enabled? != false,

do: extension

Slight optimization. Less looping, more direct filtering. Gets you list of extensions only allowed at product and device

nshoes

Looking good so far; I left some comments, questions, and concerns.

nshoes · 2024-11-19T17:49:36Z

lib/nerves_hub/devices.ex

+  def enable_extension_setting(%Device{} = device, extension_string) do
+    device = Repo.get(Device, device.id)
+
+    Device.changeset(device, %{"extensions" => %{extension_string => true}})


It does not, embeds_one raises by default on replace. I think an on_replace: :update will work here.

nshoes · 2024-11-19T17:50:54Z

config/test.exs

@@ -87,6 +87,8 @@ config :nerves_hub, NervesHub.SwooshMailer, adapter: Swoosh.Adapters.Test

 config :nerves_hub, NervesHub.RateLimit, limit: 100

+config :sentry, environment_name: :test


What does this do? Were we reporting errors to Sentry from tests before? 😨

nshoes · 2024-11-19T17:52:15Z

lib/nerves_hub/extensions/health.ex

+  # @impl NervesHub.Extensions
+  def init(socket) do
+    # Allow DB settings?
+    socket
+  end


nshoes · 2024-11-19T17:53:43Z

lib/nerves_hub/extensions/health.ex

+
+    health_interval =
+      case get_in(extension_config, [:health, :interval_minutes]) do
+        i when is_integer(i) -> i


Is this needed if we're calling String.to_integer() in runtime.exs?

nshoes · 2024-11-19T17:56:11Z

lib/nerves_hub/products.ex

+  def enable_extension_setting(%Product{} = product, extension_string) do
+    product = Repo.get(Product, product.id)
+
+    Product.changeset(product, %{"extensions" => %{extension_string => true}})
+    |> Repo.update()
+    |> tap(fn _ ->
+      topic = "product:#{product.id}:extensions"
+
+      NervesHubWeb.DeviceEndpoint.broadcast(topic, "attach", %{"extensions" => [extension_string]})
+    end)
+  end
+
+  def disable_extension_setting(%Product{} = product, extension_string) do
+    product = Repo.get(Product, product.id)
+
+    Product.changeset(product, %{"extensions" => %{extension_string => false}})
+    |> Repo.update()
+    |> tap(fn _ ->
+      topic = "product:#{product.id}:extensions"
+
+      NervesHubWeb.DeviceEndpoint.broadcast(topic, "detach", %{"extensions" => [extension_string]})
+    end)


Thoughts on refactoring this into a toggle_extension_setting vs handling each condition explicitly?

nshoes · 2024-11-19T18:01:32Z

test/nerves_hub_web/channels/device_channel_test.exs

  end

  describe "device location" do
+    # This test needs rework due to the extensions mechanism
+    @tag :pending


I see you did some work on this test, is anything more needed?

nshoes · 2024-11-19T18:02:26Z

test/support/socket_client.ex

      |> assign(:reply, reply)

    {:ok, socket}
  end

  @impl true
+  def handle_message("device", "extensions:get", _message, socket) do
+    {:ok, socket}


Why do we do nothing here?

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

jjcarstens force-pushed the features-channel branch from 61b9720 to e14326c Compare September 2, 2024 18:24

jjcarstens force-pushed the features-channel branch from e14326c to 2963f8d Compare September 27, 2024 03:05

lawik reviewed Sep 27, 2024

View reviewed changes

lib/nerves_hub_web/channels/device_channel.ex Outdated Show resolved Hide resolved

lawik marked this pull request as ready for review October 28, 2024 15:57

lawik force-pushed the features-channel branch 2 times, most recently from 8ea0780 to 1db8da2 Compare October 28, 2024 20:17

jjcarstens force-pushed the features-channel branch 2 times, most recently from b8e62e9 to 5de29ff Compare November 11, 2024 22:13

lawik marked this pull request as draft November 19, 2024 13:01

lawik force-pushed the features-channel branch 2 times, most recently from 410d841 to e2c441d Compare November 19, 2024 14:31

lawik force-pushed the features-channel branch from e2c441d to ede38df Compare November 19, 2024 14:56

lawik removed the request for review from oestrich November 19, 2024 14:56

lawik marked this pull request as ready for review November 19, 2024 14:56

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

jjcarstens commented Nov 19, 2024

View reviewed changes

nshoes reviewed Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add device extensions support #1479

Add device extensions support #1479

jjcarstens commented Sep 1, 2024 •

edited by lawik

Loading

lawik left a comment

lawik commented Oct 28, 2024

lawik commented Nov 11, 2024

lawik commented Nov 12, 2024

lawik commented Nov 12, 2024

lawik commented Nov 19, 2024

lawik commented Nov 19, 2024

lawik commented Nov 19, 2024

jjcarstens Nov 19, 2024

jjcarstens Nov 19, 2024

jjcarstens Nov 19, 2024

nshoes Nov 19, 2024

jjcarstens Nov 19, 2024

jjcarstens Nov 19, 2024

jjcarstens Nov 19, 2024

jjcarstens Nov 19, 2024

jjcarstens Nov 19, 2024

nshoes left a comment

nshoes Nov 19, 2024

nshoes Nov 19, 2024

nshoes Nov 19, 2024

nshoes Nov 19, 2024

nshoes Nov 19, 2024

nshoes Nov 19, 2024

nshoes Nov 19, 2024

		@@ -87,6 +87,8 @@ config :nerves_hub, NervesHub.SwooshMailer, adapter: Swoosh.Adapters.Test

		config :nerves_hub, NervesHub.RateLimit, limit: 100

		config :sentry, environment_name: :test

Add device extensions support #1479

Are you sure you want to change the base?

Add device extensions support #1479

Conversation

jjcarstens commented Sep 1, 2024 • edited by lawik Loading

lawik left a comment

Choose a reason for hiding this comment

lawik commented Oct 28, 2024

lawik commented Nov 11, 2024

lawik commented Nov 12, 2024

lawik commented Nov 12, 2024

lawik commented Nov 19, 2024

lawik commented Nov 19, 2024

lawik commented Nov 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nshoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjcarstens commented Sep 1, 2024 •

edited by lawik

Loading