-
-
Notifications
You must be signed in to change notification settings - Fork 20
fix traceroute/ improve dest parser #127
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
Conversation
|
|
| { | ||
| var p1 = (uint)Random.Shared.Next(); | ||
| var p2 = (uint)Random.Shared.Next(); | ||
| var id = unchecked(p1 + p2); |
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.
Obviously outside of the scope of this PR, but I've thought about consolidating the ID generation into a base factory or helper of some variety.
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.
move it to PacketUtils.GenerateRandomPacketId. In this MR, I did not change the code of other components (e.g. PositionMessageFactory)
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 fixes critical issues in the traceroute command implementation and enhances the destination argument parser to accept multiple input formats. The traceroute fixes include correcting ID generation (previously producing only 0xFF values), improving response matching logic, and adding backward route and SNR information display. The argument parser improvements enable users to specify node addresses in hexadecimal (with ! or 0x prefix) or decimal integer formats.
Key Changes
- Fixed traceroute packet ID generation and added Priority.Reliable flag
- Enhanced response matching to verify RequestId and Portnum for proper traceroute responses
- Added backward route and SNR display with proper role inversion for received traceroute data
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| Meshtastic/Data/MessageFactories/TraceRouteMessageFactory.cs | Modified ID generation from problematic formula to sum of two random values; added Priority field |
| Meshtastic.Cli/Utilities/ArgumentParsers.cs | New parser for node IDs supporting hex (! and 0x prefix) and decimal formats |
| Meshtastic.Test/Utilities/ArgumentParsersTests.cs | Test suite for new ArgumentParsers with various format test cases |
| Meshtastic.Cli/Program.cs | Integrated new NodeIdParser for --dest option |
| Meshtastic.Cli/Display/ProtobufPrinter.cs | Refactored route display to show both toward/backward routes with SNR info |
| Meshtastic.Cli/CommandHandlers/TraceRouteCommandHandler.cs | Improved response matching using RequestId and Portnum validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| var nodeStr = result.Tokens[0].Value.ToLowerInvariant(); | ||
|
|
||
| var hexMatch = Regex.Match(nodeStr, "^(!|0x)(?<num>([a-f|0-9][a-f|0-9]){1,4})$"); |
Copilot
AI
Dec 11, 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 regex pattern requires hex strings to have an even number of characters (pairs) with the pattern ([a-f|0-9][a-f|0-9]){1,4}, which means it only accepts 2, 4, 6, or 8 hex digits. This rejects valid hex values with odd-length strings like "0xabc" or "!f". Consider changing the pattern to [a-f0-9]{1,8} to accept any hex string from 1 to 8 characters, which would properly parse all valid uint32 hex values.
| var hexMatch = Regex.Match(nodeStr, "^(!|0x)(?<num>([a-f|0-9][a-f|0-9]){1,4})$"); | |
| var hexMatch = Regex.Match(nodeStr, "^(!|0x)(?<num>[a-f0-9]{1,8})$"); |
|
|
||
| var nodeStr = result.Tokens[0].Value.ToLowerInvariant(); | ||
|
|
||
| var hexMatch = Regex.Match(nodeStr, "^(!|0x)(?<num>([a-f|0-9][a-f|0-9]){1,4})$"); |
Copilot
AI
Dec 11, 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 regex pattern uses pipe characters inside a character class incorrectly. The pattern [a-f|0-9] will match 'a' through 'f', the literal pipe character '|', or '0' through '9'. This should be [a-f0-9] or [0-9a-f] instead to match hexadecimal digits without including the pipe character as a valid match.
| var hexMatch = Regex.Match(nodeStr, "^(!|0x)(?<num>([a-f|0-9][a-f|0-9]){1,4})$"); | |
| var hexMatch = Regex.Match(nodeStr, "^(!|0x)(?<num>([a-f0-9][a-f0-9]){1,4})$"); |
Traceroute command:
idgeneration (was alwaysFF)destarg parser:intandhexformats