From 349d4e77ec65ad1763dd5419ddd00923b81397ee Mon Sep 17 00:00:00 2001 From: Josh Kalderimis Date: Sat, 8 Feb 2025 21:55:05 +1300 Subject: [PATCH] Use the new firmware update status messages These new accepted Hub messages hook deeper into the updating lifecycle, as well as fixing issues where new firmware aren't sent to devices when an update is ignored or encounters errors. This also moves the responsibility for rescheduling the update to Hub, which will resend the update shortly after the date sent by Link. --- lib/nerves_hub_link.ex | 10 +-- lib/nerves_hub_link/client.ex | 5 +- lib/nerves_hub_link/socket.ex | 51 +++++++++--- lib/nerves_hub_link/update_manager.ex | 84 ++++++++++---------- test/nerves_hub_link/update_manager_test.exs | 7 +- 5 files changed, 93 insertions(+), 64 deletions(-) diff --git a/lib/nerves_hub_link.ex b/lib/nerves_hub_link.ex index 04023e04..b529d37d 100644 --- a/lib/nerves_hub_link.ex +++ b/lib/nerves_hub_link.ex @@ -55,14 +55,14 @@ defmodule NervesHubLink do @doc """ Send update progress percentage for display in web """ - @spec send_update_progress(non_neg_integer()) :: :ok - defdelegate send_update_progress(progress), to: Socket + @spec send_firmware_update_progress(non_neg_integer()) :: :ok + defdelegate send_firmware_update_progress(progress), to: Socket @doc """ - Send an update status to web + Send a firmware status update status to hub """ - @spec send_update_status(String.t() | atom()) :: :ok - defdelegate send_update_status(status), to: Socket + @spec send_firmware_update_status(status :: atom(), payload :: map()) :: :ok + defdelegate send_firmware_update_status(status, payload \\ %{}), to: Socket @doc """ Send a file to the connected console diff --git a/lib/nerves_hub_link/client.ex b/lib/nerves_hub_link/client.ex index 7a03d548..acc7cc9e 100644 --- a/lib/nerves_hub_link/client.ex +++ b/lib/nerves_hub_link/client.ex @@ -174,12 +174,13 @@ defmodule NervesHubLink.Client do # TODO: nasty side effects here. Consider moving somewhere else case data do {:progress, percent} -> - NervesHubLink.send_update_progress(percent) + NervesHubLink.send_firmware_update_progress(percent) {:error, _, message} -> - NervesHubLink.send_update_status("fwup error #{message}") + NervesHubLink.send_firmware_update_status(:error, %{reason: message}) {:ok, 0, _message} -> + NervesHubLink.send_firmware_update_status(:finalizing) initiate_reboot() _ -> diff --git a/lib/nerves_hub_link/socket.ex b/lib/nerves_hub_link/socket.ex index e3c4a4f0..0e9d47be 100644 --- a/lib/nerves_hub_link/socket.ex +++ b/lib/nerves_hub_link/socket.ex @@ -29,14 +29,17 @@ defmodule NervesHubLink.Socket do GenServer.cast(__MODULE__, :reconnect) end - @spec send_update_progress(non_neg_integer()) :: :ok - def send_update_progress(progress) do - GenServer.cast(__MODULE__, {:send_update_progress, progress}) + @spec send_firmware_update_progress(percent_progress :: non_neg_integer()) :: :ok + def send_firmware_update_progress(percent_progress) do + GenServer.cast( + __MODULE__, + {:send_firmware_update_status, :progress, %{percent: percent_progress}} + ) end - @spec send_update_status(String.t()) :: :ok - def send_update_status(status) do - GenServer.cast(__MODULE__, {:send_update_status, status}) + @spec send_firmware_update_status(status :: atom(), payload :: map()) :: :ok + def send_firmware_update_status(status, payload \\ %{}) do + GenServer.cast(__MODULE__, {:send_firmware_update_status, status, payload}) end @spec check_connection(atom()) :: boolean() @@ -270,13 +273,39 @@ defmodule NervesHubLink.Socket do {:noreply, disconnect(socket)} end - def handle_cast({:send_update_progress, progress}, socket) do - _ = push(socket, @device_topic, "fwup_progress", %{value: progress}) + def handle_cast({:send_firmware_update_progress, :progress, progress}, socket) do + _ = + push(socket, @device_topic, "firmware_update_status", %{ + status: "progress", + percent: progress + }) + + {:noreply, socket} + end + + def handle_cast({:send_firmware_update_status, :finalizing, payload}, socket) do + {:ok, push_ref} = + push( + socket, + @device_topic, + "firmware_update_status", + Map.merge(payload, %{status: "applying"}) + ) + + _ = await_reply(push_ref) + {:noreply, socket} end - def handle_cast({:send_update_status, status}, socket) do - _ = push(socket, @device_topic, "status_update", %{status: status}) + def handle_cast({:send_firmware_update_status, status, payload}, socket) do + _ = + push( + socket, + @device_topic, + "firmware_update_status", + Map.merge(payload, %{status: to_string(status)}) + ) + {:noreply, socket} end @@ -366,7 +395,7 @@ defmodule NervesHubLink.Socket do into: %{}, do: {name, to_string(ver)} - {:ok, join(socket, "extensions", available_extensions)} + {:ok, join(socket, @extensions_topic, available_extensions)} end ## diff --git a/lib/nerves_hub_link/update_manager.ex b/lib/nerves_hub_link/update_manager.ex index ccb000ea..5ea9c1fa 100644 --- a/lib/nerves_hub_link/update_manager.ex +++ b/lib/nerves_hub_link/update_manager.ex @@ -23,13 +23,18 @@ defmodule NervesHubLink.UpdateManager do @type status :: :idle - | {:fwup_error, String.t()} - | :update_rescheduled | {:updating, integer()} + | :applying + + @type previous_status :: + :ignored + | :rescheduled + | :successful + | {:error, String.t()} @type t :: %__MODULE__{ status: status(), - update_reschedule_timer: nil | :timer.tref(), + previous_status: previous_status(), download: nil | GenServer.server(), fwup: nil | GenServer.server(), fwup_config: FwupConfig.t(), @@ -37,7 +42,7 @@ defmodule NervesHubLink.UpdateManager do } defstruct status: :idle, - update_reschedule_timer: nil, + previous_status: nil, fwup: nil, download: nil, fwup_config: nil, @@ -110,8 +115,8 @@ defmodule NervesHubLink.UpdateManager do _from, %State{} = state ) do - state = maybe_update_firmware(update, fwup_public_keys, state) - {:reply, state.status, state} + {result, state} = maybe_update_firmware(update, fwup_public_keys, state) + {:reply, result, state} end def handle_call(:currently_downloading_uuid, _from, %State{update_info: nil} = state) do @@ -143,16 +148,8 @@ defmodule NervesHubLink.UpdateManager do {:reply, :ok, state} end - @impl GenServer - def handle_info({:update_reschedule, response, fwup_public_keys}, state) do - {:noreply, - maybe_update_firmware(response, fwup_public_keys, %State{ - state - | update_reschedule_timer: nil - })} - end - # messages from FWUP + @impl GenServer def handle_info({:fwup, message}, state) do _ = state.fwup_config.handle_fwup_message.(message) @@ -160,21 +157,29 @@ defmodule NervesHubLink.UpdateManager do {:ok, 0, _message} -> Logger.info("[NervesHubLink] FWUP Finished") :alarm_handler.clear_alarm(NervesHubLink.UpdateInProgress) - {:noreply, %State{state | fwup: nil, update_info: nil, status: :idle}} + + {:noreply, + %State{ + state + | fwup: nil, + update_info: nil, + status: :applying, + previous_status: :successful + }} {:progress, percent} -> {:noreply, %State{state | status: {:updating, percent}}} {:error, _, message} -> :alarm_handler.clear_alarm(NervesHubLink.UpdateInProgress) - {:noreply, %State{state | status: {:fwup_error, message}}} + {:noreply, %State{state | status: :idle, previous_status: {:error, message}}} _ -> {:noreply, state} end end - @spec maybe_update_firmware(UpdateInfo.t(), [binary()], State.t()) :: State.t() + @spec maybe_update_firmware(UpdateInfo.t(), [binary()], State.t()) :: {atom(), State.t()} defp maybe_update_firmware( %UpdateInfo{} = _update_info, _fwup_public_keys, @@ -186,43 +191,42 @@ defmodule NervesHubLink.UpdateManager do # interrupt FWUP and let the task finish. After update and reboot, the # device will check-in and get an update message if it was actually new and # required - state + {:ignored, state} end defp maybe_update_firmware(%UpdateInfo{} = update_info, fwup_public_keys, %State{} = state) do - # Cancel an existing timer if it exists. - # This prevents rescheduled updates` - # from compounding. - state = maybe_cancel_timer(state) - # possibly offload update decision to an external module. # This will allow application developers # to control exactly when an update is applied. # note: update_available is a behaviour function case state.fwup_config.update_available.(update_info) do :apply -> - start_fwup_stream(update_info, fwup_public_keys, state) + NervesHubLink.send_firmware_update_status(:updating) + {{:updating, 0}, start_fwup_stream(update_info, fwup_public_keys, state)} :ignore -> - state + NervesHubLink.send_firmware_update_status(:ignored) + Logger.info("[NervesHubLink] ignoring firmware update request") + {:ignored, %{state | status: :idle, previous_status: :ignored}} - {:reschedule, ms} -> - timer = - Process.send_after(self(), {:update_reschedule, update_info, fwup_public_keys}, ms) - - Logger.info("[NervesHubLink] rescheduling firmware update in #{ms} milliseconds") - %{state | status: :update_rescheduled, update_reschedule_timer: timer} - end - end + {:reschedule, minutes, :minutes} -> + until = + DateTime.utc_now() + |> DateTime.add(minutes, :minute) - defp maybe_update_firmware(_, _, state), do: state + NervesHubLink.send_firmware_update_status(:reschedule, %{until: until}) + Logger.info("[NervesHubLink] rescheduling firmware update in #{minutes} minutes") + {:rescheduled, %{state | status: :idle, previous_status: :rescheduled}} - defp maybe_cancel_timer(%{update_reschedule_timer: nil} = state), do: state - - defp maybe_cancel_timer(%{update_reschedule_timer: timer} = state) do - _ = Process.cancel_timer(timer) + {:reschedule, ms} -> + until = + DateTime.utc_now() + |> DateTime.add(round(ms / 1_000), :second) - %{state | update_reschedule_timer: nil} + NervesHubLink.send_firmware_update_status(:reschedule, %{until: until}) + Logger.info("[NervesHubLink] rescheduling firmware update in #{ms / 1_000} seconds") + {:rescheduled, %{state | status: :idle, previous_status: :rescheduled}} + end end @spec start_fwup_stream(UpdateInfo.t(), [binary()], State.t()) :: State.t() diff --git a/test/nerves_hub_link/update_manager_test.exs b/test/nerves_hub_link/update_manager_test.exs index 53ca8ad4..724d3cd4 100644 --- a/test/nerves_hub_link/update_manager_test.exs +++ b/test/nerves_hub_link/update_manager_test.exs @@ -57,13 +57,8 @@ defmodule NervesHubLink.UpdateManagerTest do } {:ok, manager} = UpdateManager.start_link(fwup_config) - assert UpdateManager.apply_update(manager, update_payload, []) == :update_rescheduled + assert UpdateManager.apply_update(manager, update_payload, []) == :rescheduled assert_received :rescheduled - refute_received {:fwup, _} - - assert_receive {:fwup, {:progress, 0}}, 250 - assert_receive {:fwup, {:progress, 100}} - assert_receive {:fwup, {:ok, 0, ""}} end test "apply with fwup environment", %{update_payload: update_payload, devpath: devpath} do