Fix: Handle all escape characters#245
Conversation
MathiasKoch
left a comment
There was a problem hiding this comment.
Hey Kenneth, thanks for tackling the escaping — the serializer logic itself looks correct and the tests are solid.
There's a soundness issue though: the PR doesn't update AtatLen for String<T> (still 1 + T + 1 in atat/src/derive.rs:50-52), so the buffer sized from MAX_LEN can overflow when escaping expands characters. A String<10> full of backslashes would serialize to 32 bytes but the buffer only accounts for 12.
You mentioned the 3 * T + 2 fix in the PR description, and I agree that's the correct worst case — but applying it unconditionally to AtatLen::LEN penalizes every command, even those with escape_strings = false.
An alternative approach: add an ESCAPED_LEN associated constant to the AtatLen trait alongside LEN. For most types they're identical. For String<T>, LEN stays 1 + T + 1 and ESCAPED_LEN becomes 3 * T + 2. The derive macro already knows the escape_strings flag per-command, so it can pick the right constant when computing MAX_LEN:
pub trait AtatLen {
const LEN: usize;
const ESCAPED_LEN: usize;
}
// Non-string types: no difference (handled by impl_length! macro)
macro_rules! impl_length {
($type:ty, $len:expr) => {
impl AtatLen for $type {
const LEN: usize = $len;
const ESCAPED_LEN: usize = $len;
}
};
}
// String: only ESCAPED_LEN pays the 3x cost
impl<const T: usize> AtatLen for String<T> {
const LEN: usize = 1 + T + 1;
const ESCAPED_LEN: usize = 3 * T + 2;
}Wrappers propagate both (Option<T>, &T, Vec<T, L> forward ESCAPED_LEN from the inner type).
Then in atat_derive/src/len.rs, struct_len takes the escape_strings flag and emits <#ty as atat::AtatLen>::ESCAPED_LEN vs ::LEN accordingly. Same for enum_len and the &str with #[at_arg(len = N)] case (3 * N + 2 vs 1 + N + 1).
In cmd.rs, the MAX_LEN computation just uses the escaped variant when escape_strings = true. The existing cmd_len += 2 for escape_strings can be removed since quotes are already part of both LEN and ESCAPED_LEN on string types (it was double-counting before).
Net effect: commands with escape_strings = false see zero buffer overhead. Commands with escape_strings = true only inflate string fields, not the entire struct. A String<128> goes from 130 → 386 only in commands that actually use escaping.
Does that sound like a reasonable approach to you?
|
That sounds like a good idea :D |
ad669c9 to
d53fdbc
Compare
Add ESCAPED_LEN associated constant to the AtatLen trait so commands with escape_strings=true use worst-case 3x buffer for string fields, while non-escaping commands keep the original sizing. Also fix all clippy warnings across the workspace including tests.
d53fdbc to
1280263
Compare
There was a problem hiding this comment.
@KennethKnudsen97 Looks good to me now.
Ready for your review
|
I have reviewed and testet this PR. Everything is good. |
* Update embedded-io to 0.7.0 * Implement Display and Error traits for IoError and Error enums * Update dependencies and improve serialization tests * add logging to from_slice (#251) * implement AtatLen for NonZero<T> (#252) with select values for T, since it is currently unstable to implement traits on a generic NonZero<T>. * fix serialization of None option in middle of struct fields (#234) (#253) serialize_none() unconditionally removed the last written byte, which worked for a single-field struct (removing '=') but broke multi-field structs by eating the ',' delimiter before a middle None field. Now serialize_none() only removes the separator when it is the '=' sign (first field of a top-level struct with value_sep enabled). Commas for non-first fields are preserved, producing empty delimited fields (e.g. AT+CMD=0,,1). SerializeStruct now tracks the buffer position after the last field that wrote content. In end(), written is restored to that position before appending the termination, stripping trailing commas from trailing None fields. * Fix: Handle all escape characters (#245) * handle all escape characters * cargo fmt * bump heapless and embassy * bump embassy and heapless * add ESCAPED_LEN to AtatLen for escape-aware buffer sizing Add ESCAPED_LEN associated constant to the AtatLen trait so commands with escape_strings=true use worst-case 3x buffer for string fields, while non-escaping commands keep the original sizing. Also fix all clippy warnings across the workspace including tests. --------- Co-authored-by: Kenneth Sylvest Knudsen <ksk@factbird.com> Co-authored-by: Mathias <mk@factbird.com> * Implement `AtatLen` with heapless `LenT` (#254) * update heapless-bytes version and promote to workspace-dependency I think that it may be nicer to have dependency versions declared in the workspace Cargo.toml. That way, you don't end up in the situation where you forget to update a dependency in all of the crate Cargo.tomls (as seems to have happened with heapless-bytes) * impl AtatLen for generic LenT parameter in heapless String and Vec This is a new feature of heapless 0.9, which allows to use a smaller size type to save space. * Update embedded-io to 0.7.0 * Implement Display and Error traits for IoError and Error enums * Update dependencies and improve serialization tests * Update embassy deps to released crates.io versions, drop [patch] section - Remove [patch.crates-io] git overrides now that embassy 0.10.0 is released - embassy-sync: 0.7.2 → 0.8.0 - embassy-executor: 0.9.1 → 0.10.0 (arch-cortex-m → platform-cortex-m feature) - embassy-rp: 0.9.0 → 0.10.0 - atat/defmt feature: add heapless/defmt (available since heapless 0.9.2) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Restore workspace dependencies and remove unnecessary patch section * Refactor error handling to use `thiserror`, cleanup redundant implementations --------- Co-authored-by: Johann Carl Meyer <info@johannc.de> Co-authored-by: Mathias Koch <mk@blackbird.online> Co-authored-by: Kenneth Knudsen <98805797+KennethKnudsen97@users.noreply.github.com> Co-authored-by: Kenneth Sylvest Knudsen <ksk@factbird.com> Co-authored-by: Mathias <mk@factbird.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
I think I also need to change the calculation of the length of string
To worst case scenario like
This will make the size 3 times the size, only to handle the case where every character in the string need to be escaped.