Repository navigation
Conversation
e6beb29 to
683cf29
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new DeviceModel.get_models() implementation is likely broken due to mismatched hwModel typing (string vs int), which can cause valid /model/displaynames/{model_id} requests to return empty results.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces small refactor “seams” to make adding a second mesh protocol additive (rather than a rewrite): handler registration becomes protocol-namespaced, the Meshtastic.org HTTP client is renamed to avoid confusion with protobuf protocol code, and the legacy python -m bridger.mesh decoder entry point is moved into the existing CLI subcommand structure.
Changes:
- Namespace handler registration/lookup by
(protocol, message_type)with a"meshtastic"default. - Rename the Meshtastic.org HTTP client package to
bridger/device_apiand update imports. - Move the base64 packet decode utility from
bridger/mesh/__main__.pyintobridger decodeand add CLI tests.
File summaries
| File | Description |
|---|---|
| tests/test_handler.py | Updates assertions for protocol-namespaced handler registry keys; adds protocol-collision test. |
| tests/test_cli.py | Adds tests for the new decode CLI subcommand. |
| bridger/mesh/handler_registry.py | Adds DEFAULT_PROTOCOL and changes registry key to (protocol, portnum). |
| bridger/mesh/main.py | Removes the old module entry point decoder. |
| bridger/mesh/init.py | Updates handler lookup to use the new (protocol, portnum) key. |
| bridger/http.py | Switches import to the renamed DeviceModel API client. |
| bridger/device_api/init.py | Introduces renamed API client module for Meshtastic device hardware metadata. |
| bridger/cli.py | Adds decode subcommand implementing the former mesh/__main__.py behavior. |
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def decode_command(args): | ||
| import base64 | ||
|
|
||
| from google.protobuf.message import DecodeError | ||
| from meshtastic.protobuf.mqtt_pb2 import ServiceEnvelope | ||
|
|
||
| from bridger.log import file_logger, logger | ||
| from bridger.mesh import PacketProcessorError, PBPacketProcessor | ||
|
|
||
| logger.remove(file_logger) | ||
|
|
||
| try: | ||
| service_envelope = ServiceEnvelope.FromString(base64.b64decode(args.packet)) | ||
| logger.info(f"Service envelope: \n{service_envelope}") | ||
|
|
||
| processor = PBPacketProcessor(service_envelope, force_decode=True) | ||
| logger.info(f"Decoded packet: \n{processor.payload_dict}") | ||
| logger.info(f"Data: {processor.data}") | ||
| except PacketProcessorError as e: | ||
| logger.warning(f"Error processing packet: {e}") | ||
| except DecodeError as e: | ||
| logger.exception(f"Error decoding message: {e}") | ||
| except Exception as e: | ||
| logger.exception(f"Error processing packet: {e}") |
| # Keyed on (protocol, message_type). Meshtastic PortNum values are small integers, so keying | ||
| # on the bare portnum would collide with any second protocol's message-type enum. | ||
| HANDLER_MAP = defaultdict(list) |
683cf29 to
140d520
Compare
c3f9825 to
c5b23d6
Compare
c5b23d6 to
fa888a3
Compare
fa888a3 to
204d7fc
Compare
204d7fc to
11870b5
Compare
11870b5 to
dabf5f6
Compare
Three cheap changes now so that adding a second mesh protocol is additive rather than a rewrite. Deliberately not the full package move: bridger/mesh -> bridger/protocols/meshtastic touches a dozen files for no benefit until the second protocol exists, and with these in place it becomes a mechanical git mv with no design decisions left. Renames bridger/meshtastic to bridger/device_api. It is an HTTP client for api.meshtastic.org, while the actual protocol code lives in bridger/mesh -- inverted naming that also shadowed the real meshtastic package for anyone reading. One class, one importer. Keys HANDLER_MAP on (protocol, message_type) instead of the bare portnum. Meshtastic PortNum values are small integers and would collide with any MeshCore message-type enum. Handlers get "meshtastic" by default, so no handler class changes. Retires bridger/mesh/__main__.py into `bridger.cli decode`. It was the one entry point that the eventual package move would have broken, and the CLI already has an argparse subcommand structure and test coverage. It also no longer opens an InfluxDB connection it never used. Not doing the MessageDecoder extraction from BridgerMQTT that this could also have included. Designing that interface with only one implementation in hand would be guessing at what MeshCore needs, and on_message is thin enough to restructure later. The genuinely reusable pieces are already protocol-agnostic: PacketDeduplicator takes (gateway_id, packet_id), and the Influx writer works purely off dataclass metadata. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
dabf5f6 to
495f7bd
Compare
Three cheap changes now so that adding a second mesh protocol is additive
rather than a rewrite. Deliberately not the full package move: bridger/mesh ->
bridger/protocols/meshtastic touches a dozen files for no benefit until the
second protocol exists, and with these in place it becomes a mechanical git mv
with no design decisions left.
Renames bridger/meshtastic to bridger/device_api. It is an HTTP client for
api.meshtastic.org, while the actual protocol code lives in bridger/mesh --
inverted naming that also shadowed the real meshtastic package for anyone
reading. One class, one importer.
Keys HANDLER_MAP on (protocol, message_type) instead of the bare portnum.
Meshtastic PortNum values are small integers and would collide with any
MeshCore message-type enum. Handlers get "meshtastic" by default, so no handler
class changes.
Retires bridger/mesh/main.py into
bridger.cli decode. It was the oneentry point that the eventual package move would have broken, and the CLI
already has an argparse subcommand structure and test coverage. It also no
longer opens an InfluxDB connection it never used.
Not doing the MessageDecoder extraction from BridgerMQTT that this could also
have included. Designing that interface with only one implementation in hand
would be guessing at what MeshCore needs, and on_message is thin enough to
restructure later. The genuinely reusable pieces are already protocol-agnostic:
PacketDeduplicator takes (gateway_id, packet_id), and the Influx writer works
purely off dataclass metadata.
Co-Authored-By: Claude Opus 5 (1M context) [email protected]
Part of stack #267, created with GitHub Stacks CLI. Review and merge bottom-up.
🤖 Generated with Claude Code
Note
Low Risk
Mostly naming, registry keying, and CLI relocation with tests; existing Meshtastic handlers keep default protocol behavior with no production path changes beyond the device API import.
Overview
Prepares the codebase for a second mesh protocol (e.g. MeshCore) with three targeted changes.
bridger/meshtastic→bridger/device_api: The HTTP client for api.meshtastic.org is renamed so it no longer clashes with the realmeshtasticpackage or with protocol code underbridger/mesh.bridger/http.pynow importsDeviceModelfrombridger.device_api.Protocol-aware handler registry:
HANDLER_MAPis keyed by(protocol, message_type)instead of port number alone, avoiding collisions when another protocol reuses small integer message types. Handlers default to"meshtastic"viaDEFAULT_PROTOCOL; optionalprotocolon handler classes registers under another namespace.PBPacketProcessorlooks up handlers through a newhandler_keyproperty.Decode debugging via CLI: The standalone
bridger/mesh/__main__.pyentry point is removed and replaced bybridger decode <base64-packet>, which decodes aServiceEnvelopewithPBPacketProcessorwithout opening an unused InfluxDB client. Tests cover successful decode, bad base64 handling, and subcommand wiring.Reviewed by Cursor Bugbot for commit 495f7bd. Bugbot is set up for automated code reviews on this repo. Configure here.