Fix wrong end of frame marker

[heavyrubberslave]

Summary by CodeRabbit

• Refactor
  • Standardized device/serial output so all command responses, status lines and errors consistently terminate with a single line terminator.
  • Unified output handling across protocol commands (introduce, attributes, status, get, set, unrecognized) for more predictable framing.
  • Streamlined output API to use a single send-style interface, removing the previous automatic newline path.

[coderabbitai]

📝 Walkthrough

Walkthrough

The pull request replaces the print/println output API with a send-based API and a defined ETX terminator. ISlvCtrlOut gained send(...) overloads; protocol response paths now use send(...) and append ETX. SlvCtrlArduinoOut methods were renamed/forwarded from print(...) to send(...), and println(...) was removed.

Changes

Cohort / File(s)	Summary
Protocol Interface src/SlvCtrlProtocol.h	Replaced output API usages to call send(...) overloads and added static constexpr const char ETX = '\n';. Added virtual send(...) declarations to ISlvCtrlOut. Updated all protocol output paths (introduce, attributes, status, get, set, unrecognized) to terminate with ETX instead of using println.
Transport Implementation src/SlvCtrlArduinoSerialCommandsTransport.h	Renamed/added send(...) overrides in SlvCtrlArduinoOut for char, const char*, int32_t, uint32_t, float, and bool, forwarding to underlying s_.print(...). Removed println(const char*) override.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• Memory optimizations #2: Modifies the same output/interface files (ISlvCtrlOut / SlvCtrlArduinoOut) and print/send overloads; likely directly related to this output API refactor.

Poem

🐇 I nibble bytes and hop on send,

replacing prints that used to end.
ETX now signs each little line,
neat and final—oh how fine!
A tiny rabbit clap for the new design.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix wrong end of frame marker' directly aligns with the main change: replacing the print/println interface with send() methods and introducing ETX as the proper end-of-frame marker throughout the codebase.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/end-of-frame-marker

---

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

src/SlvCtrlProtocol.h (3)

274-274: ETX naming may be misleading — '\n' is LF (0x0A), not ASCII ETX (0x03).

The standard ASCII ETX character is 0x03 (End of Text). Here ETX is defined as '\n' (Line Feed, 0x0A). If this is an intentional protocol-level "end of transmission" marker (just named ETX for convenience), consider a brief comment clarifying this, or rename to something like FRAME_END or EOL to avoid confusion with ASCII ETX.

✏️ Suggested clarification

-    static constexpr const char ETX = '\n';
+    static constexpr const char FRAME_END = '\n';  // end-of-frame marker (LF)

Or, if you want to keep the name ETX, add a comment:

-    static constexpr const char ETX = '\n';
+    static constexpr const char ETX = '\n';  // protocol end-of-frame (LF, not ASCII ETX 0x03)

---

335-350: Many send() calls per response line — consider reducing call overhead for frequently invoked commands.

Each protocol response involves many small send() calls (e.g., cmdGet success path makes 7 calls). On an Arduino with serial output, each s_.print() call can involve non-trivial overhead. For hot paths, composing the response in a small stack buffer and flushing once could reduce overhead.

This is purely a performance consideration for the future and not blocking.

---

338-338: Long compound statements reduce readability.

Lines 338, 341, 342, 372, and 373 pack multiple send() calls and an early return into a single line. While functional, these are hard to read and debug. Consider splitting into multiple lines.

✏️ Example for line 338

-      if (!name) { o.send("get;;status:error,reason:missing_attribute_name_arg"); o.send(ETX); return; }
+      if (!name) {
+        o.send("get;;status:error,reason:missing_attribute_name_arg");
+        o.send(ETX);
+        return;
+      }

src/SlvCtrlArduinoSerialCommandsTransport.h (1)

31-33: Default argument on virtual function override.

Both ISlvCtrlOut::send(float, uint8_t decimals = 3) and this override specify decimals = 3. This works correctly when the default values match, but in C++ the default is resolved based on the static type of the pointer/reference. If the defaults ever diverge, callers through ISlvCtrlOut& would silently use the base class default. This is a well-known C++ pitfall — not a bug today, but worth being aware of.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}