Fix for double confirmed downlink (#1012)

helium · Oct 11, 2023 · 53122ca · 53122ca
1 parent 9ec9296
commit 53122ca
Show file tree

Hide file tree

Showing 2 changed files with 114 additions and 35 deletions.
diff --git a/src/device/router_device_worker.erl b/src/device/router_device_worker.erl
@@ -1548,12 +1548,27 @@ validate_frame(
     FrameCache,
     OfferCache
 ) ->
-    <<MType:3, _MHDRRFU:3, _Major:2, _DevAddr:4/binary, _ADR:1, _ADRACKReq:1, _ACK:1, _RFU:1,
+    <<MType:3, _MHDRRFU:3, _Major:2, _DevAddr:4/binary, _ADR:1, _ADRACKReq:1, ACK:1, _RFU:1,
         _FOptsLen:4, _FCnt:16, _FOpts:_FOptsLen/binary,
         _PayloadAndMIC/binary>> = blockchain_helium_packet_v1:payload(Packet),
 
     DeviceFCnt = router_device:fcnt(Device0),
 
+    %% This only applies when it's the first packet we see
+    %% If frame countain ACK=1 we should clear message from queue and go on next
+    QueueDeviceUpdates =
+        case {ACK, router_device:queue(Device0)} of
+            %% Check if acknowledging confirmed downlink
+            {1, [#downlink{confirmed = true} | T]} ->
+                [{queue, T}, {fcntdown, router_device:fcntdown_next_val(Device0)}];
+            {1, _} ->
+                lager:warning("got ack when no confirmed downlinks in queue"),
+                [];
+            _ ->
+                []
+        end,
+    QueueUpdatedDevice = router_device:update(QueueDeviceUpdates, Device0),
+
     case MType of
         MType when MType == ?CONFIRMED_UP orelse MType == ?UNCONFIRMED_UP ->
             FrameAck = router_utils:mtype_to_ack(MType),
@@ -1576,7 +1591,7 @@ validate_frame(
                         Packet,
                         PubKeyBin,
                         Region,
-                        Device0,
+                        QueueUpdatedDevice,
                         OfferCache,
                         false
                     );
@@ -1625,7 +1640,7 @@ validate_frame(
                         Packet,
                         PubKeyBin,
                         Region,
-                        Device0,
+                        QueueUpdatedDevice,
                         OfferCache,
                         false
                     )
@@ -1703,22 +1718,7 @@ validate_frame_(PacketFCnt, Packet, PubKeyBin, HotspotRegion, Device0, OfferCach
                         {region, Region},
                         {last_known_datarate, DRIdx}
                     ],
-                    %% If frame countain ACK=1 we should clear message from queue and go on next
-                    QueueDeviceUpdates =
-                        case {ACK, router_device:queue(Device0)} of
-                            %% Check if acknowledging confirmed downlink
-                            {1, [#downlink{confirmed = true} | T]} ->
-                                [{queue, T}, {fcntdown, router_device:fcntdown_next_val(Device0)}];
-                            {1, _} ->
-                                lager:warning("got ack when no confirmed downlinks in queue"),
-                                [];
-                            _ ->
-                                []
-                        end,
-                    Device1 = router_device:update(
-                        QueueDeviceUpdates ++ BaseDeviceUpdates,
-                        Device0
-                    ),
+                    Device1 = router_device:update(BaseDeviceUpdates, Device0),
                     Frame = #frame{
                         mtype = MType,
                         devaddr = DevAddr,
@@ -1769,22 +1769,7 @@ validate_frame_(PacketFCnt, Packet, PubKeyBin, HotspotRegion, Device0, OfferCach
                         {region, Region},
                         {last_known_datarate, DRIdx}
                     ],
-                    %% If frame countain ACK=1 we should clear message from queue and go on next
-                    QueueDeviceUpdates =
-                        case {ACK, router_device:queue(Device0)} of
-                            %% Check if acknowledging confirmed downlink
-                            {1, [#downlink{confirmed = true} | T]} ->
-                                [{queue, T}, {fcntdown, router_device:fcntdown_next_val(Device0)}];
-                            {1, _} ->
-                                lager:warning("got ack when no confirmed downlinks in queue"),
-                                [];
-                            _ ->
-                                []
-                        end,
-                    Device1 = router_device:update(
-                        QueueDeviceUpdates ++ BaseDeviceUpdates,
-                        Device0
-                    ),
+                    Device1 = router_device:update(BaseDeviceUpdates, Device0),
                     Frame = #frame{
                         mtype = MType,
                         devaddr = DevAddr,

diff --git a/test/router_downlink_SUITE.erl b/test/router_downlink_SUITE.erl
@@ -10,6 +10,7 @@
 ]).
 
 -export([
+    the_k3nt_test/1,
     http_downlink_test/1,
     console_tool_downlink_test/1,
     console_tool_downlink_order_test/1,
@@ -57,6 +58,7 @@ groups() ->
 
 all_tests() ->
     [
+        the_k3nt_test,
         http_downlink_test,
         console_tool_downlink_test,
         console_tool_downlink_order_test,
@@ -89,6 +91,98 @@ end_per_testcase(TestCase, Config) ->
 %% TEST CASES
 %%--------------------------------------------------------------------
 
+%% AKA double donwlink queued before join
+the_k3nt_test(Config) ->
+    Payload1 = <<"httpdownlink1">>,
+    Message1 = #{payload_raw => base64:encode(Payload1), region => <<"US915">>},
+    Payload2 = <<"httpdownlink2">>,
+    Message2 = #{payload_raw => base64:encode(Payload2), region => <<"US915">>},
+    %% Sending debug event from websocket
+    WSPid =
+        receive
+            {websocket_init, P} -> P
+        after 2500 -> ct:fail(websocket_init_timeout)
+        end,
+    WSPid ! {downlink, Message1},
+    WSPid ! {downlink, Message2},
+
+    ok = test_utils:wait_until(fun() ->
+        case router_device_cache:get(?CONSOLE_DEVICE_ID) of
+            {error, not_found} -> false;
+            {ok, Device} -> 2 == length(router_device:queue(Device))
+        end
+    end),
+
+    #{
+        pubkey_bin := PubKeyBin,
+        stream := Stream,
+        hotspot_name := _HotspotName
+    } = test_utils:join_device(Config),
+
+    %% Waiting for reply from router to hotspot
+    test_utils:wait_state_channel_message(1250),
+
+    {ok, Device0} = router_device_cache:get(?CONSOLE_DEVICE_ID),
+    Stream !
+        {send,
+            test_utils:frame_packet(
+                ?UNCONFIRMED_UP,
+                PubKeyBin,
+                router_device:nwk_s_key(Device0),
+                router_device:app_s_key(Device0),
+                0
+            )},
+
+    %% Waiting for donwlink message on the hotspot
+    {ok, _} = test_utils:wait_state_channel_message(
+        {false, 1, Payload1},
+        Device0,
+        Payload1,
+        ?UNCONFIRMED_DOWN,
+        1,
+        0,
+        1,
+        0
+    ),
+
+    ok = test_utils:wait_until(fun() ->
+        case router_device_cache:get(?CONSOLE_DEVICE_ID) of
+            {error, not_found} -> false;
+            {ok, Device} -> 1 == length(router_device:queue(Device))
+        end
+    end),
+
+    Stream !
+        {send,
+            test_utils:frame_packet(
+                ?UNCONFIRMED_UP,
+                PubKeyBin,
+                router_device:nwk_s_key(Device0),
+                router_device:app_s_key(Device0),
+                1
+            )},
+
+    %% Waiting for donwlink message on the hotspot
+    {ok, _} = test_utils:wait_state_channel_message(
+        {false, 1, Payload2},
+        Device0,
+        Payload2,
+        ?UNCONFIRMED_DOWN,
+        0,
+        0,
+        1,
+        1
+    ),
+
+    ok = test_utils:wait_until(fun() ->
+        case router_device_cache:get(?CONSOLE_DEVICE_ID) of
+            {error, not_found} -> false;
+            {ok, Device} -> 0 == length(router_device:queue(Device))
+        end
+    end),
+
+    ok.
+
 http_downlink_test(Config) ->
     Payload = <<"httpdownlink">>,
     Message = #{payload_raw => base64:encode(Payload)},