-
Notifications
You must be signed in to change notification settings - Fork 0
Add message path to ping command response #28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add message path to ping command response #28
Conversation
This commit adds comprehensive debug logging to the _on_message_received method to capture all available fields in message event payloads. This is an investigation step to determine if path/routing information is available in message payloads. The logging captures: - All keys in the payload dictionary - Full payload contents - Event object type and attributes - Event __dict__ for additional fields This will help determine if we can include message path information in ping responses without needing to correlate separate PATH_UPDATE events.
This commit adds network diagnostic capabilities to MeshBot using the meshcore_py library's ping and trace commands. Changes: 1. MeshCoreInterface: - Added send_trace() abstract method and implementations - Added path_len field to MeshCoreMessage dataclass - Extract path_len from message payloads (number of hops) - Implemented send_trace in both Mock and Real interfaces 2. Network Tools (new file): - Created src/meshbot/tools/network.py - ping_node tool: Check node connectivity (uses send_statusreq) - trace_path tool: Send trace packets for routing diagnostics - Both tools provide clear success/failure messages 3. System Prompt: - Updated prompts/default.md to document ping/trace capabilities - Clarified that these are actual network diagnostics, not text responses 4. Tool Registration: - Updated tools/__init__.py to register network_tools The implementation allows users to: - Ping specific mesh nodes to check connectivity - Send trace packets to diagnose mesh network routing - View path_len (hop count) in message logs Based on meshcore-cli and meshcore-mqtt documentation for send_trace and send_statusreq commands.
This commit improves the trace_path tool to wait for and display actual trace responses instead of just confirming the packet was sent. Changes to MeshCoreInterface: - Added _trace_responses asyncio.Queue to collect trace responses - Subscribe to EventType.TRACE_DATA for trace responses - Added _on_trace_response() handler to process trace events - Implemented send_trace_and_wait() method: - Sends trace packet - Waits up to 10s (configurable) for responses - Collects all responses received within timeout - Returns list of trace response payloads Changes to network tools: - Updated trace_path tool to use send_trace_and_wait() - Added timeout parameter (default: 10s) - Formats trace responses showing: - Number of hops - Node IDs for each hop - Latency in milliseconds - Improved error messages for no responses or failures Mock implementation includes simulated trace responses for testing. This addresses the requirement to wait for trace responses before responding to the user, providing actual routing diagnostics rather than just acknowledgment of packet transmission.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds network diagnostic capabilities to MeshBot by introducing ping and trace tools for mesh network operations. The changes enable the bot to perform actual network diagnostics like checking node connectivity and tracing routing paths through the mesh network. The PR also adds path length tracking to messages to understand how many hops messages take through the network.
Key changes:
- Added new
ping_nodeandtrace_pathdiagnostic tools that interact with the mesh network - Implemented
send_traceandsend_trace_and_waitmethods in the MeshCore interface with support for both mock and real implementations - Extended
MeshCoreMessagedataclass to track path length (number of hops) for received messages
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/meshbot/tools/network.py | New module implementing ping_node and trace_path diagnostic tools for mesh network troubleshooting |
| src/meshbot/tools/init.py | Updated to register the new network diagnostic tools |
| src/meshbot/meshcore_interface.py | Added path_len field to MeshCoreMessage; implemented send_trace/send_trace_and_wait methods in both mock and real interfaces; added trace response handling with event subscription |
| prompts/default.md | Updated bot prompt to clarify that ping/trace commands trigger actual network diagnostics rather than text responses |
Comments suppressed due to low confidence (1)
src/meshbot/meshcore_interface.py:989
- This import of module asyncio is redundant, as it was previously imported on line 3.
import asyncio
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Simulate mock trace responses | ||
| responses = [ | ||
| {"hop": 0, "node": "local", "latency_ms": 0}, | ||
| {"hop": 1, "node": "node1", "latency_ms": 45}, | ||
| {"hop": 2, "node": "node2", "latency_ms": 92}, | ||
| ] |
Copilot
AI
Nov 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mock trace response format doesn't match what the trace_path tool expects. The tool in network.py lines 88-93 expects responses with a "path" key containing a list of steps with "hash" and "snr" fields, but these mock responses have "hop", "node", and "latency_ms" fields instead. This will cause the trace_path tool to display empty results when used with the mock interface.
Consider updating the mock responses to match the expected format:
responses = [
{"path": [
{"hash": "local", "snr": 0},
{"hash": "node1", "snr": 15.5},
{"hash": "node2", "snr": 12.3}
]}
]| # Simulate mock trace responses | |
| responses = [ | |
| {"hop": 0, "node": "local", "latency_ms": 0}, | |
| {"hop": 1, "node": "node1", "latency_ms": 45}, | |
| {"hop": 2, "node": "node2", "latency_ms": 92}, | |
| ] | |
| # Simulate mock trace responses in expected format | |
| responses = [ | |
| { | |
| "path": [ | |
| {"hash": "local", "snr": 0}, | |
| {"hash": "node1", "snr": 15.5}, | |
| {"hash": "node2", "snr": 12.3}, | |
| ] | |
| } | |
| ] | |
| ] |
| for i, response in enumerate(responses): | ||
| step_path = response.get("path", []) | ||
| for n, step in enumerate(step_path): | ||
| hash = step.get("hash") | ||
| snr = step.get("snr", "unknown") | ||
| if hash: | ||
| msg += f"{n + 1}. {hash} (SNR: {snr})\n" |
Copilot
AI
Nov 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nested loop structure processes response.get("path", []) for each response and iterates through the path steps, but the outer loop variable i is never used. If multiple responses each contain their own path, this could result in duplicated or confusing output.
Consider clarifying the logic:
- If each response represents a separate trace result, label them accordingly (e.g., "Trace result 1:", "Trace result 2:")
- If only one response is expected, consider simplifying to just process
responses[0] - If responses represent incremental updates to the same path, clarify this in the comments
| self._own_public_key: Optional[str] = None | ||
| self._own_node_name: Optional[str] = None | ||
| self._message_handlers: List[Callable[[MeshCoreMessage], Any]] = [] | ||
| self._trace_responses: asyncio.Queue[Dict[str, Any]] = asyncio.Queue() |
Copilot
AI
Nov 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The asyncio.Queue is instantiated without calling it as a function. This should be asyncio.Queue() with parentheses to actually create a queue instance. Without the parentheses, _trace_responses will be a reference to the Queue class itself, not an instance, which will cause AttributeErrors when trying to call .put(), .get(), or .empty() methods.
| """Ping a mesh node to check connectivity and measure latency. | ||
| This sends a status request to the specified node and waits for a response. | ||
| Use this to test if a node is reachable and measure round-trip time. |
Copilot
AI
Nov 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring states "check connectivity and measure latency" and "measure round-trip time", but the function only returns a boolean success/failure and doesn't actually measure or report any latency or round-trip time information. Consider either:
- Updating the docstring to remove mentions of latency measurement: "Ping a mesh node to check connectivity."
- Enhancing the function to actually measure and return latency information if that's a desired feature.
| """Ping a mesh node to check connectivity and measure latency. | |
| This sends a status request to the specified node and waits for a response. | |
| Use this to test if a node is reachable and measure round-trip time. | |
| """Ping a mesh node to check connectivity. | |
| This sends a status request to the specified node and waits for a response. | |
| Use this to test if a node is reachable. |
No description provided.