skf cleanup 2 (#976)

* when a device is found to be deleted, remove all pairs * use the router_device helper * always send skf adds through the filter (#979) * usort from the start
helium · Jul 11, 2023 · dd6f8c3 · dd6f8c3
1 parent 12df903
commit dd6f8c3
Show file tree

Hide file tree

Showing 3 changed files with 27 additions and 46 deletions.
diff --git a/src/apis/router_console_ws_worker.erl b/src/apis/router_console_ws_worker.erl
@@ -401,12 +401,8 @@ update_device_record(DB, CF, DeviceID) ->
             case router_device_cache:get(DeviceID) of
                 {ok, Device} ->
                     catch router_ics_eui_worker:remove([DeviceID]),
-                    case router_device:devaddr_int_nwk_key(Device) of
-                        {ok, {DevAddrInt, NwkSKey}} ->
-                            catch router_ics_skf_worker:update([{remove, DevAddrInt, NwkSKey, 0}]);
-                        {error, _} ->
-                            ok
-                    end;
+                    Removes = router_device:make_skf_removes(Device),
+                    catch router_ics_skf_worker:update(Removes);
                 _ ->
                     ok
             end,
@@ -477,25 +473,19 @@ update_device_record(DB, CF, DeviceID) ->
             case router_device:devaddr_int_nwk_key(Device) of
                 {error, _} ->
                     ok;
-                {ok, {DevAddrInt, NwkSKey}} ->
+                {ok, {_DevAddrInt, _NwkSKey}} ->
                     case {OldIsActive, IsActive} of
                         %% end state is active, multi-buy has changed.
                         {_, true} when OldMultiBuy =/= NewMultiBuy ->
-                            catch router_ics_skf_worker:update([
-                                {add, DevAddrInt, NwkSKey, NewMultiBuy}
-                            ]),
+                            catch router_ics_skf_worker:add_device_ids([DeviceID]),
                             lager:debug("device active, multi-buy changed, sent SKF add");
                         %% inactive -> active.
                         {false, true} ->
-                            catch router_ics_skf_worker:update([
-                                {add, DevAddrInt, NwkSKey, NewMultiBuy}
-                            ]),
+                            catch router_ics_skf_worker:add_device_ids([DeviceID]),
                             lager:debug("device un-paused, sent SKF add");
                         %% active -> inactive.
                         {true, false} ->
-                            catch router_ics_skf_worker:update([
-                                {remove, DevAddrInt, NwkSKey, OldMultiBuy}
-                            ]),
+                            catch router_ics_skf_worker:remove_device_ids([DeviceID]),
                             lager:debug("device paused, sent SKF remove");
                         %% active state has not changed, multi-buy remains the same
                         _ ->

diff --git a/src/device/router_device.erl b/src/device/router_device.erl
@@ -50,7 +50,7 @@
     credentials_to_evict/1,
     limit_credentials/1,
     devaddr_int_nwk_key/1,
-    make_skf_removes/1, make_skf_removes/2
+    make_skf_removes/1, make_skf_removes/2, make_skf_removes/3
 ]).
 
 %% ------------------------------------------------------------------
@@ -551,12 +551,20 @@ make_skf_removes(Device) ->
     DevAddrs :: list(binary())
 ) -> [{remove, non_neg_integer(), binary(), non_neg_integer()}].
 make_skf_removes(NwkKeys, DevAddrs) ->
+    make_skf_removes(NwkKeys, DevAddrs, 0).
+
+-spec make_skf_removes(
+    NwkKeys :: list({binary() | undefined, binary() | undefined}),
+    DevAddrs :: list(binary()),
+    MultiBuy :: non_neg_integer()
+) -> [{remove, non_neg_integer(), binary(), non_neg_integer()}].
+make_skf_removes(NwkKeys, DevAddrs, MultiBuy) ->
     DevAddrToInt = fun(D) ->
         <<Int:32/integer-unsigned-big>> = lorawan_utils:reverse(D),
         Int
     end,
 
-    [{remove, DevAddrToInt(D), NSK, 0} || {NSK, _} <- NwkKeys, D <- DevAddrs].
+    lists:usort([{remove, DevAddrToInt(D), NSK, MultiBuy} || {NSK, _} <- NwkKeys, D <- DevAddrs]).
 
 %% ------------------------------------------------------------------
 %% RocksDB Device Functions

diff --git a/src/device/router_device_worker.erl b/src/device/router_device_worker.erl
@@ -366,13 +366,8 @@ handle_cast(
     case router_console_api:get_device(DeviceID) of
         {error, not_found} ->
             catch router_ics_eui_worker:remove([DeviceID]),
-            ok =
-                case router_device:devaddr_int_nwk_key(Device0) of
-                    {ok, {DevAddrInt, NwkSKey}} ->
-                        catch router_ics_skf_worker:update([{remove, DevAddrInt, NwkSKey, 0}]);
-                    _ ->
-                        ok
-                end,
+            Removes = router_device:make_skf_removes(Device0),
+            catch router_ics_skf_worker:update(Removes),
             %% Important to remove details about the device _before_ the device.
             ok = router_device:delete(DB, CF, DeviceID),
             ok = router_device_cache:delete(DeviceID),
@@ -440,25 +435,19 @@ handle_cast(
             case router_device:devaddr_int_nwk_key(Device1) of
                 {error, _} ->
                     ok;
-                {ok, {DevAddrInt, NwkSKey}} ->
+                {ok, {_DevAddrInt, _NwkSKey}} ->
                     case {OldIsActive, IsActive} of
                         %% end state is active, multi-buy has changed.
                         {_, true} when OldMultiBuy =/= NewMultiBuy ->
-                            catch router_ics_skf_worker:update([
-                                {add, DevAddrInt, NwkSKey, NewMultiBuy}
-                            ]),
+                            catch router_ics_skf_worker:add_device_ids([DeviceID]),
                             lager:debug("device active, multi-buy changed, sent SKF add");
                         %% inactive -> active.
                         {false, true} ->
-                            catch router_ics_skf_worker:update([
-                                {add, DevAddrInt, NwkSKey, NewMultiBuy}
-                            ]),
+                            catch router_ics_skf_worker:add_device_ids([DeviceID]),
                             lager:debug("device un-paused, sent SKF add");
                         %% active -> inactive.
                         {true, false} ->
-                            catch router_ics_skf_worker:update([
-                                {remove, DevAddrInt, NwkSKey, OldMultiBuy}
-                            ]),
+                            catch router_ics_skf_worker:remove_device_ids([DeviceID]),
                             lager:debug("device paused, sent SKF remove");
                         %% active state has not changed, multi-buy remains the same
                         _ ->
@@ -797,17 +786,11 @@ handle_cast(
                         MultiBuy = maps:get(multi_buy, router_device:metadata(D1), 0),
                         ToAdd = [{add, DevAddr0, UsedNwkSKey, MultiBuy}],
 
-                        DevAddrToInt = fun(D) ->
-                            <<Int:32/integer-unsigned-big>> = lorawan_utils:reverse(D),
-                            Int
-                        end,
-
-                        %% We have to usort just in case DevAddr assigned is the same
-                        ToRemove0 = lists:usort([
-                            {remove, DevAddrToInt(DevAddr), NwkSKey, MultiBuy}
-                         || {NwkSKey, _} <- Keys, DevAddr <- router_device:devaddrs(Device0)
-                        ]),
-
+                        ToRemove0 = router_device:make_skf_removes(
+                            Keys,
+                            router_device:devaddrs(Device0),
+                            MultiBuy
+                        ),
                         %% Making sure that the pair that was added is not getting removed (just in case DevAddr assigned is the same)
                         ToRemove1 = ToRemove0 -- [{remove, DevAddr0, UsedNwkSKey, MultiBuy}],
                         ok = router_ics_skf_worker:update(ToAdd ++ ToRemove1),