From 1239426dc7b2479bae76f1e4cfa556dbf9ed5cfe Mon Sep 17 00:00:00 2001 From: "Walter A. Boring IV" Date: Fri, 28 Aug 2026 16:11:36 -0400 Subject: [PATCH] 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 | 9 ++++ aprsd/packets/core.py | 13 +++++- aprsd/threads/rx.py | 9 ++++ tests/packets/test_packet.py | 53 +++++++++++++++++++++ tests/threads/test_rx.py | 89 ++++++++++++++++++++++++++++++++++++ 5 files changed, 171 insertions(+), 2 deletions(-) diff --git a/ChangeLog.md b/ChangeLog.md index 8596795..596aac0 100644 --- a/ChangeLog.md +++ b/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) diff --git a/aprsd/packets/core.py b/aprsd/packets/core.py index cb77325..220c78e 100644 --- a/aprsd/packets/core.py +++ b/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( diff --git a/aprsd/threads/rx.py b/aprsd/threads/rx.py index 2485b16..66b0e1c 100644 --- a/aprsd/threads/rx.py +++ b/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, ), ) diff --git a/tests/packets/test_packet.py b/tests/packets/test_packet.py index c188154..d7e3755 100644 --- a/tests/packets/test_packet.py +++ b/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) diff --git a/tests/threads/test_rx.py b/tests/threads/test_rx.py index 4c18059..3145067 100644 --- a/tests/threads/test_rx.py +++ b/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}', + )