Skip to content

Remove AtatLen#260

Merged
MathiasKoch merged 1 commit into
FactbirdHQ:masterfrom
Rimpampa:len
Jul 2, 2026
Merged

Remove AtatLen#260
MathiasKoch merged 1 commit into
FactbirdHQ:masterfrom
Rimpampa:len

Conversation

@Rimpampa

Copy link
Copy Markdown
Contributor

I don't see how it could be used since even when calling AtatCmd::write the buffer size depends on the client's INGRESS_BUF_SIZE and no check is made by default.

I made this PR just to see if it would compile, if others are actually using this feature then fell free to close it.

@Rimpampa

Copy link
Copy Markdown
Contributor Author

@MathiasKoch any thoughts on this?

@MathiasKoch

Copy link
Copy Markdown
Member

Thanks for digging into this. You're right that it's effectively unused.

The premise holds: atat's own client never reads MAX_LEN, since it writes into the caller-provided buf with no bounds check. The only thing that actually consumes the computed value is 3 test-only stack-buffer allocations in ublox-short-range-rs (command/edm/mod.rs:216, 259, 395, all inside #[cfg(test)]). Everything else, meaning the MAX_LEN propagation impls and the manual/derived AtatLen impls in both ublox drivers, is just plumbing feeding a value nothing ultimately reads. No production code path uses it.

So I'm happy to merge this as-is. The drivers are rev-pinned to atat, so nothing breaks until someone bumps the rev, and the consumer-side cleanup is small (drop the dead AtatLen impls/derives, resize 3 test buffers). We can do that in a follow-up when we update the pin.

LGTM.

@Rimpampa
Rimpampa marked this pull request as ready for review June 26, 2026 14:05
@Rimpampa

Copy link
Copy Markdown
Contributor Author

Nice, then I suggest to take a look just at the atat_derive/src/enum_.rs file since I had to change the Default impl for the AtatEnum derive macro, but other than that I think it's ready to be merged.

@MathiasKoch

Copy link
Copy Markdown
Member

Sorry about the delay on this! It has been a busy week.

@MathiasKoch
MathiasKoch merged commit d1a1dd5 into FactbirdHQ:master Jul 2, 2026
8 checks passed
Rimpampa added a commit to Rimpampa/atat that referenced this pull request Jul 2, 2026
MathiasKoch pushed a commit that referenced this pull request Jul 2, 2026
JcBernack pushed a commit to rocknitive/atat that referenced this pull request Jul 15, 2026
(cherry picked from commit d1a1dd5)

# Conflicts:
#	atat/src/derive.rs
#	atat_derive/src/cmd.rs
#	atat_derive/src/len.rs
#	atat_derive/src/lib.rs
JcBernack pushed a commit to rocknitive/atat that referenced this pull request Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants