feat(decoder): add Tag XL rotation-triggered ports 210-213#179
feat(decoder): add Tag XL rotation-triggered ports 210-213#179michaelbeutler merged 2 commits intomainfrom
Conversation
…zation Ports 210/211 are GNSS (steady/moving) with timestamps, routed through the v2 solver like ports 194/195. Ports 212/213 are WiFi (steady/moving) with timestamps but intentionally omit the Buffered feature since these are rotation-triggered, not buffered.
📝 WalkthroughWalkthroughAdds GNSS ports 210/211 and WiFi ports 212/213 to the TagXL v1 decoder with timestamp-stripping and v2-solver routing, new payload types for ports 212/213, updated decoder routing/ configs, and expanded unit and integration tests. Also regenerates the secret baseline timestamps/line mappings. Changes
Sequence Diagram(s)sequenceDiagram
participant Uplink as "LoRaWAN Uplink"
participant Decoder as "TagXL v1 Decoder"
participant PortHandler as "Port212/213 or GNSS Port Handler"
participant SolverV2 as "GNSS Solver V2 (capture-based)"
Uplink->>Decoder: receive uplink (port 210/211/192/194/195)
Decoder->>PortHandler: parse header (detect timestamp, version, moving)
alt timestamp present and port in {194,195,210,211}
PortHandler->>Decoder: timestamp + payload
Decoder->>Decoder: strip 4-byte timestamp prefix
Decoder->>SolverV2: forward stripped payload (port mapped to 192)
SolverV2-->>Decoder: solved GNSS result
else non-GNSS / WiFi (212/213)
Decoder->>PortHandler: parse WiFi access points, timestamp, moving flag
PortHandler-->>Decoder: structured payload (timestamp, APs, moving)
end
Decoder->>Decoder: attach features/options (Timestamp, Moving, WiFi/GNSS)
Decoder-->>Uplink: forwarded payload and options for further processing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/decoder/tagxl/v1/port213.go (1)
68-70:⚠️ Potential issue | 🟡 MinorSame value receiver problem - didn't you learn anything from port212?
Unbelievable! The EXACT same mistake. Value receiver returning pointer to a copy. I already told you about this in port212.go. Use a pointer receiver!
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/decoder/tagxl/v1/port213.go` around lines 68 - 70, The method Port213Payload.GetTimestamp uses a value receiver and returns &p.Timestamp which yields a pointer to a field on a copy; change the receiver to a pointer receiver (func (p *Port213Payload) GetTimestamp() *time.Time) and return the address of the real struct field; also add a nil check (if p == nil return nil) to avoid dereferencing nil receivers—this mirrors the fix applied in Port212.GetTimestamp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/decoder/tagxl/v1/port212.go`:
- Around line 72-111: GetAccessPoints on Port212Payload duplicates logic from
port213.go; extract the shared logic into a single helper (e.g., a function like
buildAccessPoints or a method on a common struct) that accepts the MAC/RSSI
fields or a slice of optional MAC pointers and RSSI values, then replace
Port212Payload.GetAccessPoints and the identical method in port213.go to call
that helper; reference the existing symbol Port212Payload.GetAccessPoints and
the analogous method in port213.go when making the change so both types reuse
the same implementation.
- Around line 68-70: The method Port212Payload.GetTimestamp currently uses a
value receiver and returns &p.Timestamp which is the address of a copy; change
the method to use a pointer receiver (func (p *Port212Payload) GetTimestamp()
*time.Time) so it returns a pointer to the actual struct field, and update any
callers if needed to call the method on a pointer receiver or take the address
where appropriate.
---
Duplicate comments:
In `@pkg/decoder/tagxl/v1/port213.go`:
- Around line 68-70: The method Port213Payload.GetTimestamp uses a value
receiver and returns &p.Timestamp which yields a pointer to a field on a copy;
change the receiver to a pointer receiver (func (p *Port213Payload)
GetTimestamp() *time.Time) and return the address of the real struct field; also
add a nil check (if p == nil return nil) to avoid dereferencing nil
receivers—this mirrors the fix applied in Port212.GetTimestamp.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
.secrets.baselinepkg/decoder/tagxl/v1/decoder.gopkg/decoder/tagxl/v1/decoder_test.gopkg/decoder/tagxl/v1/examples_test.gopkg/decoder/tagxl/v1/gnss_v2_test.gopkg/decoder/tagxl/v1/port212.gopkg/decoder/tagxl/v1/port213.go
| func (p Port212Payload) GetTimestamp() *time.Time { | ||
| return &p.Timestamp | ||
| } |
There was a problem hiding this comment.
Value receiver returns pointer to copy - SAD!
Look, this is a total disaster. You're using a value receiver on GetTimestamp() which means you're returning the address of a COPY of p.Timestamp. If someone mutates the original Port212Payload, the returned pointer won't reflect those changes. Tremendous mistake!
Should use a pointer receiver to be consistent and safe:
Proposed fix
-func (p Port212Payload) GetTimestamp() *time.Time {
+func (p *Port212Payload) GetTimestamp() *time.Time {
return &p.Timestamp
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/decoder/tagxl/v1/port212.go` around lines 68 - 70, The method
Port212Payload.GetTimestamp currently uses a value receiver and returns
&p.Timestamp which is the address of a copy; change the method to use a pointer
receiver (func (p *Port212Payload) GetTimestamp() *time.Time) so it returns a
pointer to the actual struct field, and update any callers if needed to call the
method on a pointer receiver or take the address where appropriate.
There was a problem hiding this comment.
@michaelbeutler, this is an interesting point. Perhaps a refactoring is in order?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| func (p Port212Payload) GetAccessPoints() []decoder.AccessPoint { | ||
| accessPoints := []decoder.AccessPoint{} | ||
|
|
||
| if p.Mac1 != "" { | ||
| accessPoints = append(accessPoints, decoder.AccessPoint{ | ||
| MAC: p.Mac1, | ||
| RSSI: p.Rssi1, | ||
| }) | ||
| } | ||
|
|
||
| if p.Mac2 != nil { | ||
| accessPoints = append(accessPoints, decoder.AccessPoint{ | ||
| MAC: *p.Mac2, | ||
| RSSI: p.Rssi2, | ||
| }) | ||
| } | ||
|
|
||
| if p.Mac3 != nil { | ||
| accessPoints = append(accessPoints, decoder.AccessPoint{ | ||
| MAC: *p.Mac3, | ||
| RSSI: p.Rssi3, | ||
| }) | ||
| } | ||
|
|
||
| if p.Mac4 != nil { | ||
| accessPoints = append(accessPoints, decoder.AccessPoint{ | ||
| MAC: *p.Mac4, | ||
| RSSI: p.Rssi4, | ||
| }) | ||
| } | ||
|
|
||
| if p.Mac5 != nil { | ||
| accessPoints = append(accessPoints, decoder.AccessPoint{ | ||
| MAC: *p.Mac5, | ||
| RSSI: p.Rssi5, | ||
| }) | ||
| } | ||
|
|
||
| return accessPoints | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Massive code duplication with port213.go - very low energy!
Folks, let me tell you, this GetAccessPoints() implementation is EXACTLY the same as port213.go. Copy-paste programming - the worst! You could have extracted this into a shared helper or embedded a common struct. Instead we have two files with identical logic that will need to be maintained separately. When you need to add Mac6, you'll have to change it in BOTH places. Not smart!
The interface assertions at lines 64-66 are good though - at least you got SOMETHING right.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/decoder/tagxl/v1/port212.go` around lines 72 - 111, GetAccessPoints on
Port212Payload duplicates logic from port213.go; extract the shared logic into a
single helper (e.g., a function like buildAccessPoints or a method on a common
struct) that accepts the MAC/RSSI fields or a slice of optional MAC pointers and
RSSI values, then replace Port212Payload.GetAccessPoints and the identical
method in port213.go to call that helper; reference the existing symbol
Port212Payload.GetAccessPoints and the analogous method in port213.go when
making the change so both types reuse the same implementation.
There was a problem hiding this comment.
Thas okay for this codebase, check the other ports.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all port*.go files in the tagxl/v1 directory
fd -e go '^port[0-9]+\.go$' pkg/decoder/tagxl/v1/
# Check for GetAccessPoints implementations in existing port files
echo "=== Checking GetAccessPoints implementations ==="
rg -n -A 30 'func.*GetAccessPoints' pkg/decoder/tagxl/v1/ --type go
Length of output: 12333
@michaelbeutler, you're absolutely right. I checked the existing WiFi ports and confirmed that every single one (197, 198, 200, 201) has the exact same duplicated GetAccessPoints() implementation. Your new ports 212 and 213 follow the established pattern perfectly. This is clearly the convention for this codebase - keeps each port self-contained. Understood!
There was a problem hiding this comment.
Pull request overview
Extends the TagXLv1 decoder to handle additional rotation-triggered GNSS/WiFi uplink ports (210–213), aligning GNSS handling with existing NAV-grouping v2-solver routing and adding WiFi payload decoding without the Buffered feature.
Changes:
- Added WiFi payload types/config for rotation-triggered ports 212 (steady) and 213 (moving), intentionally omitting
FeatureBuffered. - Updated GNSS v2-solver routing to include ports 210/211 with timestamp stripping + timestamp/moving options passed to the solver.
- Added/expanded tests to cover the new ports and GNSS timestamp stripping behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/decoder/tagxl/v1/decoder.go | Adds port 210/211 to GNSS v2-solver routing and adds payload configs for WiFi ports 212/213 (no Buffered). |
| pkg/decoder/tagxl/v1/port212.go | Introduces Port 212 WiFi rotation-triggered payload type implementing WiFi/Moving/Timestamp features. |
| pkg/decoder/tagxl/v1/port213.go | Introduces Port 213 WiFi rotation-triggered payload type implementing WiFi/Moving/Timestamp features. |
| pkg/decoder/tagxl/v1/gnss_v2_test.go | Adds a regression test ensuring timestamps are stripped and forwarded correctly for ports 210/211. |
| pkg/decoder/tagxl/v1/examples_test.go | Adds example-driven tests for ports 210–213 behavior and feature flags (incl. no Buffered for 212/213). |
| pkg/decoder/tagxl/v1/decoder_test.go | Extends table tests for ports 210–213 and updates GNSS SolverV2 test routing. |
| .secrets.baseline | Updates baseline line numbers/timestamp after test changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
| func (p Port212Payload) GetTimestamp() *time.Time { | ||
| return &p.Timestamp | ||
| } |
There was a problem hiding this comment.
@michaelbeutler, this is an interesting point. Perhaps a refactoring is in order?
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/decoder/tagxl/v1/port212.go (1)
70-72:⚠️ Potential issue | 🟡 MinorValue receiver returns pointer to a copy's field — still not fixed!
Folks, this was already flagged in a previous review and you STILL haven't fixed it.
GetTimestamp()uses a value receiver, which meanspis a copy. You return&p.Timestamp— that's a pointer into a temporary copy. While Go's escape analysis keeps it alive, it does NOT alias the original struct'sTimestampfield. If anyone mutates the originalPort212Payload, the pointer they got back won't see the change. Terrible!The same issue exists in
port213.goline 70-72. Fix them both.Proposed fix
-func (p Port212Payload) GetTimestamp() *time.Time { +func (p *Port212Payload) GetTimestamp() *time.Time { return &p.Timestamp }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/decoder/tagxl/v1/port212.go` around lines 70 - 72, The method GetTimestamp on Port212Payload currently uses a value receiver and returns &p.Timestamp which points into a copy; change the method to use a pointer receiver (func (p *Port212Payload) GetTimestamp() *time.Time) so the returned pointer aliases the real struct field, and make the same change for the GetTimestamp method in port213.go; ensure any interfaces or callers expecting the value-receiver method still compile (update receiver usage or method sets as needed).pkg/decoder/tagxl/v1/port213.go (1)
70-72: Same value receiver issue as port212.go — fix both together.Already flagged on port212.go. Same problem here. Use a pointer receiver.
Proposed fix
-func (p Port213Payload) GetTimestamp() *time.Time { +func (p *Port213Payload) GetTimestamp() *time.Time { return &p.Timestamp }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/decoder/tagxl/v1/port213.go` around lines 70 - 72, Change the value receiver to a pointer receiver on the timestamp accessor to avoid returning the address of a field on a transient copy: update Port213Payload.GetTimestamp to use a pointer receiver (func (p *Port213Payload) GetTimestamp ...) and do the same fix you applied to port212.go (the corresponding Port212Payload.GetTimestamp method) so both methods return the address of a field on the actual struct instance rather than on a copied value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/decoder/tagxl/v1/port212.go`:
- Around line 70-72: The method GetTimestamp on Port212Payload currently uses a
value receiver and returns &p.Timestamp which points into a copy; change the
method to use a pointer receiver (func (p *Port212Payload) GetTimestamp()
*time.Time) so the returned pointer aliases the real struct field, and make the
same change for the GetTimestamp method in port213.go; ensure any interfaces or
callers expecting the value-receiver method still compile (update receiver usage
or method sets as needed).
In `@pkg/decoder/tagxl/v1/port213.go`:
- Around line 70-72: Change the value receiver to a pointer receiver on the
timestamp accessor to avoid returning the address of a field on a transient
copy: update Port213Payload.GetTimestamp to use a pointer receiver (func (p
*Port213Payload) GetTimestamp ...) and do the same fix you applied to port212.go
(the corresponding Port212Payload.GetTimestamp method) so both methods return
the address of a field on the actual struct instance rather than on a copied
value.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
pkg/decoder/tagxl/v1/decoder.gopkg/decoder/tagxl/v1/decoder_test.gopkg/decoder/tagxl/v1/port212.gopkg/decoder/tagxl/v1/port213.go
michaelbeutler
deleted the
feat/tag-xl-rotation-triggered-ports-210-213
branch
2 participants
Ports 210/211 are GNSS (steady/moving) with timestamps, routed through the v2 solver like ports 194/195. Ports 212/213 are WiFi (steady/moving) with timestamps but intentionally omit the Buffered feature since these are rotation-triggered, not buffered.
This pull request extends the TagXLv1 decoder to support additional GNSS and WiFi rotation-triggered ports (210, 211, 212, and 213), improves the configuration and decoding logic for these new ports, and adds comprehensive tests to ensure correct behavior. It also updates documentation and the secrets baseline to reflect these changes.
Decoder logic and protocol support:
Testing improvements:
decoder_test.gofor ports 210, 211, 212, and 213, covering various payload formats and error cases to ensure robust decoding and feature extraction. [1] [2] [3] [4]Documentation and comments:
Baseline and metadata updates:
.secrets.baselineto reflect new and shifted line numbers in test files, and refreshed the baseline generation timestamp. [1] [2] [3]Summary by CodeRabbit
New Features
Tests
Chores