fix: clarify _send_packet()/_send_ack() scheduling contract (#263) (#273)

Replace misleading bare 'pass' with explicit comment explaining that
PacketTrack polling is handled by PacketSendSchedulerThread /
AckSendSchedulerThread.  Restructure condition to
'if not (scheduler and scheduler.is_alive()):' so the fallback
thread is only started when the scheduler is genuinely unavailable.

Closes #244

ChangeLog.md

@@ -8,6 +8,8 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog).
 
 ##### Bug Fixes
 
+- feat: send piggyback ACK (Reply-Ack) in outbound MessagePackets per APRS spec replyacks.txt [`09e66e0`](https://github.com/craigerl/aprsd/commit/09e66e0)
+
 - Fix USMetarPlugin.process() self-assignment no-op 'fromcall = fromcall' [`44ff252`](https://github.com/craigerl/aprsd/commit/44ff252)
 
 - Enable ruff isort (I) ruleset in pyproject.toml [`a3801f6`](https://github.com/craigerl/aprsd/commit/a3801f6)
@@ -26,8 +28,15 @@ Generated by [`auto-changelog`](https://github.com/CookPete/auto-changelog).
 
 - Fix APRSDStats.stats() typo 'loging_queue' → 'logging_queue' [`56839e4`](https://github.com/craigerl/aprsd/commit/56839e4)
 
+<<<<<<< HEAD
 - Fix APRSDThreadList.pause_all/unpause_all: pass lock arg to @wrapt.synchronized [`bd6f74e`](https://github.com/craigerl/aprsd/commit/bd6f74e)
 
+
+
+
+||||||| parent of 90681ca (fix: clarify _send_packet()/_send_ack() scheduling contract (#263))
+=======
+>>>>>>> 9f602f7 (fix: clarify _send_packet()/_send_ack() scheduling contract (#263))
 - Add reset() to @singleton decorator; update tests to use ClassName.reset() instead of ClassName.instance = None [`ef19aaa`](https://github.com/craigerl/aprsd/commit/ef19aaa)
 
 - Fix PacketTrack.keys/items/values — return list snapshots instead of live dict views outside the lock [`088436e`](https://github.com/craigerl/aprsd/commit/088436e)

aprsd/packets/core.py

@@ -250,10 +250,19 @@ class MessagePacket(Packet):
 
     def _build_payload(self):
         if self.msgNo:
-            self.payload = ':{}:{}{{{}'.format(
+            if self.ackMsgNo:
+                # Per http://www.aprs.org/aprs11/replyacks.txt, the Reply-Ack
+                # wire format is "text{MM}AA" where MM is our 2-char outbound
+                # sequence number and AA is the piggyback ack (the sender's
+                # msgNo we are acknowledging).
+                suffix = f'{{{self.msgNo}}}{self.ackMsgNo}'
+            else:
+                # Standard APRS message format: "text{XXXXX"
+                suffix = f'{{{self.msgNo}'
+            self.payload = ':{}:{}{}'.format(
                 self.to_call.ljust(9),
                 self._filter_for_send(self.message_text).rstrip('\n'),
-                str(self.msgNo),
+                suffix,
             )
         else:
             self.payload = ':{}:{}'.format(

aprsd/threads/rx.py

@@ -330,6 +330,11 @@ class APRSDPluginProcessPacketThread(APRSDProcessPacketThread):
         else:
             to_call = None
 
+        # Capture the sender's msgNo so we can embed it as a piggyback ACK
+        # (Reply-Ack per http://www.aprs.org/aprs11/replyacks.txt) in any
+        # plain-string reply MessagePacket we build here.
+        reply_ack = packet.msgNo if packet.msgNo else None
+
         pm = plugin.PluginManager()
         try:
             results, handled = pm.run(packet)
@@ -352,6 +357,7 @@ class APRSDPluginProcessPacketThread(APRSDProcessPacketThread):
                                     from_call=CONF.callsign,
                                     to_call=from_call,
                                     message_text=subreply,
+                                    ackMsgNo=reply_ack,
                                 ),
                             )
                 elif isinstance(reply, packets.Packet):
@@ -371,6 +377,7 @@ class APRSDPluginProcessPacketThread(APRSDProcessPacketThread):
                                 from_call=CONF.callsign,
                                 to_call=from_call,
                                 message_text=reply,
+                                ackMsgNo=reply_ack,
                             ),
                         )
 
@@ -392,6 +399,7 @@ class APRSDPluginProcessPacketThread(APRSDProcessPacketThread):
                         from_call=CONF.callsign,
                         to_call=from_call,
                         message_text=message_text,
+                        ackMsgNo=reply_ack,
                     ),
                 )
         except Exception as ex:
@@ -405,6 +413,7 @@ class APRSDPluginProcessPacketThread(APRSDProcessPacketThread):
                         from_call=CONF.callsign,
                         to_call=from_call,
                         message_text=reply,
+                        ackMsgNo=reply_ack,
                     ),
                 )
 

tests/packets/test_packet.py

@@ -159,3 +159,56 @@ class TestRejectPacket(unittest.TestCase):
                 from_call=fake.FAKE_FROM_CALLSIGN,
                 to_call=fake.FAKE_TO_CALLSIGN,
             )
+
+
+class TestMessagePacketPiggybackAck(unittest.TestCase):
+    """Tests for MessagePacket Reply-Ack (piggyback ACK) support."""
+
+    def test_standard_format_no_ack(self):
+        """Standard message with msgNo uses old {XXXXX format (no piggyback)."""
+        pkt = packets.MessagePacket(
+            from_call='W1AW',
+            to_call='KJ4ERJ',
+            message_text='hello',
+            msgNo='42',
+        )
+        pkt.prepare()
+        self.assertIn('{42', pkt.raw)
+        # Must NOT have a closing brace (old format)
+        self.assertNotIn('{42}', pkt.raw)
+
+    def test_reply_ack_format(self):
+        """MessagePacket with ackMsgNo uses new {MM}AA wire format."""
+        pkt = packets.MessagePacket(
+            from_call='W1AW',
+            to_call='KJ4ERJ',
+            message_text='hello',
+            msgNo='42',
+            ackMsgNo='HQ',
+        )
+        pkt.prepare()
+        self.assertIn('{42}HQ', pkt.raw)
+
+    def test_reply_ack_parsed_by_aprslib(self):
+        """aprslib must parse back both msgNo and ackMsgNo from a Reply-Ack packet."""
+        pkt = packets.MessagePacket(
+            from_call='W1AW',
+            to_call='KJ4ERJ',
+            message_text='test msg',
+            msgNo='AB',
+            ackMsgNo='HQ',
+        )
+        pkt.prepare()
+        parsed = aprslib.parse(pkt.raw)
+        self.assertEqual(parsed['msgNo'], 'AB')
+        self.assertEqual(parsed['ackMsgNo'], 'HQ')
+
+    def test_no_msgNo_no_ack_suffix(self):
+        """MessagePacket without msgNo produces no {…} suffix at all."""
+        pkt = packets.MessagePacket(
+            from_call='W1AW',
+            to_call='KJ4ERJ',
+            message_text='no number',
+        )
+        pkt._build_payload()
+        self.assertNotIn('{', pkt.payload)

tests/threads/test_rx.py

@@ -396,3 +396,92 @@ class TestAPRSDProcessPacketThread(unittest.TestCase):
 
             self.process_thread.process_other_packet(packet, for_us=True)
             self.assertEqual(mock_log.info.call_count, 2)
+
+
+class TestPluginProcessPacketPiggybackAck(unittest.TestCase):
+    """Integration tests for Reply-Ack (piggyback ACK) in APRSDPluginProcessPacketThread."""
+
+    def setUp(self):
+        from oslo_config import cfg
+
+        from aprsd import conf  # noqa: F401  - side-effect: registers oslo.config opts
+
+        self.CONF = cfg.CONF
+        self.CONF.callsign = 'W1AW'
+
+        self.packet_queue = queue.Queue()
+
+        self.client_patcher = mock.patch('aprsd.threads.rx.APRSDClient')
+        self.client_patcher.start()
+
+    def tearDown(self):
+        self.client_patcher.stop()
+
+    def test_reply_contains_ack_msg_no(self):
+        """When a plugin returns a plain string, the outbound MessagePacket
+        carries ackMsgNo equal to the incoming packet's msgNo."""
+        from aprsd import packets as aprsd_packets
+        from aprsd.threads import rx, tx
+
+        incoming = fake.fake_packet(message='ping', msg_number='HQ')
+        incoming.addresse = 'W1AW'
+        incoming.from_call = 'KJ4ERJ'
+
+        sent_packets = []
+
+        def capture_send(pkt, **kwargs):
+            sent_packets.append(pkt)
+
+        with mock.patch.object(tx, 'send', side_effect=capture_send):
+            thread = rx.APRSDPluginProcessPacketThread(self.packet_queue)
+            with mock.patch('aprsd.threads.rx.plugin') as mock_plugin_mod:
+                mock_pm = mock.MagicMock()
+                mock_pm.run.return_value = (['pong'], True)
+                mock_plugin_mod.PluginManager.return_value = mock_pm
+
+                thread.process_our_message_packet(incoming)
+
+        # At least one MessagePacket should have been sent
+        msg_pkts = [
+            p for p in sent_packets if isinstance(p, aprsd_packets.MessagePacket)
+        ]
+        self.assertTrue(len(msg_pkts) >= 1, 'Expected at least one MessagePacket reply')
+        for pkt in msg_pkts:
+            self.assertEqual(
+                pkt.ackMsgNo,
+                'HQ',
+                f'Expected ackMsgNo=HQ on reply, got {pkt.ackMsgNo!r}',
+            )
+
+    def test_no_reply_ack_when_incoming_has_no_msgNo(self):
+        """Replies should have ackMsgNo=None when the incoming packet has no msgNo."""
+        from aprsd import packets as aprsd_packets
+        from aprsd.threads import rx, tx
+
+        incoming = fake.fake_packet(message='ping')
+        incoming.msgNo = None
+        incoming.addresse = 'W1AW'
+        incoming.from_call = 'KJ4ERJ'
+
+        sent_packets = []
+
+        def capture_send(pkt, **kwargs):
+            sent_packets.append(pkt)
+
+        with mock.patch.object(tx, 'send', side_effect=capture_send):
+            thread = rx.APRSDPluginProcessPacketThread(self.packet_queue)
+            with mock.patch('aprsd.threads.rx.plugin') as mock_plugin_mod:
+                mock_pm = mock.MagicMock()
+                mock_pm.run.return_value = (['pong'], True)
+                mock_plugin_mod.PluginManager.return_value = mock_pm
+
+                thread.process_our_message_packet(incoming)
+
+        msg_pkts = [
+            p for p in sent_packets if isinstance(p, aprsd_packets.MessagePacket)
+        ]
+        for pkt in msg_pkts:
+            self.assertIsNone(
+                pkt.ackMsgNo,
+                f'Expected ackMsgNo=None on reply, got {pkt.ackMsgNo!r}',
+            )