Skip to content

feat: S08.03 slice 4 set command for in-RAM configuration injection #303

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.

Sign up for GitHub

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

Merged
DavidCozens merged 2 commits into from
May 9, 2026
Merged

feat: S08.03 slice 4 set command for in-RAM configuration injection #303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e6d76b7
feat: S08.03 slice 4 `set` command for in-RAM configuration injection
DavidCozens May 9, 2026
d229ef6
fix: address CI + CodeRabbit review feedback on slice 4
DavidCozens May 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 140 additions & 0 deletions DEVLOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,145 @@
# Dev Log

## 2026-05-09 — S08.03 slice 4 — `set` command for in-RAM configuration injection (#302)

Slice 3b.2 hardcoded the FreeRTOS example's identity, message body, and
endpoint into `Example/FreeRtos/SingleTask/main.c` because the FreeRTOS
port deliberately skips `Example/Common/ExampleCommandLine.c` (no
`getopt`). Slice 4 makes those configurable in-RAM via a `set NAME VALUE`
command on the existing interactive UART grammar — same channel as
`send` and `quit`. Field set: `hostname`, `appname`, `procid`, `msgid`,
`msg`, `host`, `port`, `facility`, `severity`. Successful mutations echo
`set name=value`; rejections print `set: invalid` (catchall) and leave
the prior value untouched. `quit` still cleanly tears down the example.

### Library-side dispatcher — host-TDD'd in 9 cycles

`Example/Common/ExampleInteractive` grew an `onSet` callback parallel to
the existing `onSwitch`. The runner splits args at first whitespace and
calls `bool onSet(name, value)`; the library doesn't know what fields
are settable. Echo / catchall live in the runner, driven by the
handler's bool return.

Strict ZOMBIES walk in `Tests/Example/ExampleInteractiveTest.cpp`,
`fmemopen`-backed `FILE*` for input, `dup`/`freopen`/`dup2` dance for
stdout capture:

1. `SetHandlerNotCalledWithQuitOnly` — drove the new parameter onto the
signature; all four `ExampleInteractive_Run` call sites pass `NULL`
for the new arg (POSIX, Threaded, Windows, FreeRTOS).
2. `SetCommandCallsHandlerOnce` — drove the dispatch branch.
3. `SetCommandPassesNameToHandler` — drove first-token extraction
(`memcpy` into a local name buffer + null-terminate).
4. `SetCommandPassesValueToHandler` — drove the rest-of-line as value.
5. `SetCommandWithoutValueGivesEmptyValue` — boundary, locked in by
passing without production change (the `space != NULL` ternary
already handled it).
6. `SetCommandWithEmbeddedSpacesPreservesValueAfterFirst` — boundary,
locked in: `strchr(args, ' ')` is by definition first-occurrence.
7. `NullSetHandlerSilentlyIgnoresSetLine` — exception, locked in by
the `onSet != NULL` guard already in the dispatcher.
8. `SetCommandPrintsEchoOnHandlerSuccess` — drove the
`if (onSet(...)) printf("set %s=%s\n", name, value);` echo.
9. `SetCommandPrintsInvalidOnHandlerFailure` — drove the
`else printf("set: invalid\n");` catchall.

Refactor pass extracted `RunWithInput` and `RunCapturingStdout` test
helpers so each test body reads as one or two lines. The
`SOLIDSYSLOG_POSIX`-gated `Tests/Example/CMakeLists.txt` block picks
up the new file alongside the existing `ExampleInteractive.c`
production source — same compile target, no separate test binary.

### FreeRTOS-side `OnSet` handler — integrator glue

`Example/FreeRtos/SingleTask/main.c` carries the field-name dispatch.
The walking-skeleton `TEST_*` strings became mutable static arrays
(`g_hostname[256]`, `g_appName[49]`, `g_processId[129]`,
`g_messageId[33]`, `g_msg[256]`, `g_host[16]`) sized to RFC 5424
maxima where applicable. A file-static `g_message` holds
`facility`/`severity` (mutated in place) plus `messageId`/`msg`
pointers targeting the mutable storage so each `SolidSyslog_Log`
sees current contents. `g_port` and `g_endpointVersion` back the
endpoint callbacks.

Validation per field:
- Identity / `msgid` / `msg` / `host`: non-empty, length within storage cap.
- `facility`: integer 0–23 (RFC 5424 facility range).
- `severity`: integer 0–7 (RFC 5424 severity range).
- `port`: integer 1–65535.

Two small file-local helpers do the heavy lifting: `TryUpdateString`
(bounds-check + `memcpy` + null-terminate) and `TryParseUInt`
(full-string `strtol` with end-pointer + non-negative check). The
`OnSet` handler is a chain of `strcmp` lookups; unknown field names
fall through to `return false` and the runner prints `set: invalid`.

### `host` is plumbed but currently a no-op on the wire

`SolidSyslogFreeRtosStaticResolver` ignores the host string and routes
via the `Create`-time IPv4 octets. Slice 4 still wires `g_host` through
`GetEndpoint` (and accepts `set host …` into the storage) so the
follow-up slice that teaches the resolver to parse dotted-quads is a
contained, mechanical change in one file. For now `set host 1.2.3.4`
echoes confirmation but doesn't change the destination IP — the issue
body documents this caveat.

### Endpoint version bump — integrator-owned

`GetEndpointVersion` reads from `g_endpointVersion`; `OnSet` increments
it when `port` mutates successfully. The library has zero coupling to
endpoint-mutation semantics — other integrators can choose not to bump
at all if their resolver doesn't care. `SolidSyslogUdpSender` re-pulls
`endpoint()` on the next Send when the version differs from its cached
value, which is what makes `set port 5515; send 1` actually hit 5515
instead of the cached resolver result.

### QEMU smoke

```text
PORT=5514 BYTES=<134>… FreeRtosExample … (default)
set hostname QemuFoo → set hostname=QemuFoo
PORT=5514 BYTES=<134>… QemuFoo … (mutated)
set port 5515 → set port=5515
PORT=5515 BYTES=<134>… QemuFoo … (new port)
set facility 99 → set: invalid
set port 99999 → set: invalid
set hostname → set: invalid (empty value)
set bogus value → set: invalid (unknown field)
quit (clean exit)
```

Both ports listened simultaneously via a Python `select()` listener.
Slice 3b.1.5's transparent ARP priming means the very first `send 1`
after cold start lands without a warm-up message; the port mutation
takes effect on the first send after the version bump.

### Pre-PR checks

- `clang-format --dry-run --Werror` on every changed file — clean.
- `ctest --preset debug` — 5/5 (`SolidSyslogTests`, `ExampleTests` 63/63
with 9 new `ExampleInteractive` tests, `SolidSyslogFreeRtosDatagramTest`,
`SolidSyslogFreeRtosStaticResolverTest`, `CmsdkUartTest`,
`OpenSslIntegrationTests`).
- `cmake --build --preset freertos-cross` — both ELFs link clean.
- QEMU smoke — see scenario above.
- Not run locally (CI's responsibility): `analyze-tidy`,
`analyze-cppcheck`, `analyze-iwyu`, `analyze-format`,
`build-windows-msvc`, BDD, OpenSSL integration.

### What this leaves for later slices

- `SolidSyslogFreeRtosStaticResolver` dotted-quad parsing so `set host`
takes effect on the wire.
- BDD harness pointed at the FreeRTOS target — slice 4 produces
deterministic confirmation lines (`set name=value`, `set: invalid`)
that a future Behave step can grep for.
- Service-thread + non-NullBuffer FreeRTOS-side wiring (CircularBuffer
+ FreeRTOS mutex).
- Real RTC-backed clock callback (still hardcoded `TEST_TIMESTAMP`).
- Flash-persistent config so settings survive reboot.
- CMake-driven stack-budget scaling so the `*32` magic number can come
back down to a measured value.

## 2026-05-09 — S08.03 slice 3b.2 — SingleTask example with SolidSyslogUdpSender + interactive command channel (#296)

Slice 3b.1's "send a hardcoded ping on link-up" smoke is replaced with a
Expand Down
26 changes: 25 additions & 1 deletion Example/Common/ExampleInteractive.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,27 @@ static void HandleSend(const char* args, const struct SolidSyslogMessage* messag
printf("Sent %d message%s\n", count, (count == 1) ? "" : "s");
}

void ExampleInteractive_Run(const struct SolidSyslogMessage* message, FILE* input, ExampleInteractiveSwitchHandler onSwitch)
static void HandleSet(const char* args, ExampleInteractiveSetHandler onSet)
{
char name[MAX_LINE_LENGTH];
const char* space = strchr(args, ' ');
size_t nameLen = (space != NULL) ? (size_t) (space - args) : strlen(args);
const char* value = (space != NULL) ? (space + 1) : "";

memcpy(name, args, nameLen);
name[nameLen] = '\0';

if (onSet(name, value))
{
printf("set %s=%s\n", name, value);
}
else
{
printf("set: invalid\n");
}
}

void ExampleInteractive_Run(const struct SolidSyslogMessage* message, FILE* input, ExampleInteractiveSwitchHandler onSwitch, ExampleInteractiveSetHandler onSet)
{
char line[MAX_LINE_LENGTH];

Expand Down Expand Up @@ -95,6 +115,10 @@ void ExampleInteractive_Run(const struct SolidSyslogMessage* message, FILE* inpu
{
onSwitch(args);
}
else if (onSet != NULL && MatchCommand(line, "set", &args))
{
HandleSet(args, onSet);
}

PrintPrompt();
}
Expand Down
5 changes: 4 additions & 1 deletion Example/Common/ExampleInteractive.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#ifndef EXAMPLEINTERACTIVE_H
#define EXAMPLEINTERACTIVE_H

#include <stdbool.h>
#include <stdio.h>

#include "ExternC.h"
Expand All @@ -10,8 +11,10 @@ struct SolidSyslogMessage;
EXTERN_C_BEGIN

typedef void (*ExampleInteractiveSwitchHandler)(const char* name);
typedef bool (*ExampleInteractiveSetHandler)(const char* name, const char* value);

void ExampleInteractive_Run(const struct SolidSyslogMessage* message, FILE* input, ExampleInteractiveSwitchHandler onSwitch);
void ExampleInteractive_Run(const struct SolidSyslogMessage* message, FILE* input, ExampleInteractiveSwitchHandler onSwitch,
ExampleInteractiveSetHandler onSet);
Comment on lines +14 to +17

@coderabbitai coderabbitai Bot May 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

Add a context parameter to the new set callback API.

The new callback currently cannot carry per-instance state without globals. Please align this new callback family with the repository callback contract by adding a void* context parameter and a paired context argument in ExampleInteractive_Run.

Proposed API shape 
-typedef bool (*ExampleInteractiveSetHandler)(const char* name, const char* value);
+typedef bool (*ExampleInteractiveSetHandler)(void* context, const char* name, const char* value);

-void ExampleInteractive_Run(const struct SolidSyslogMessage* message, FILE* input, ExampleInteractiveSwitchHandler onSwitch,
-                            ExampleInteractiveSetHandler onSet);
+void ExampleInteractive_Run(const struct SolidSyslogMessage* message, FILE* input, ExampleInteractiveSwitchHandler onSwitch,
+                            ExampleInteractiveSetHandler onSet, void* context);

As per coding guidelines: “For new callbacks, the function pointer must take a void* context parameter; config struct exposes paired context field; when several callbacks form a logical feature, share one context field.”

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
typedef bool (*ExampleInteractiveSetHandler)(const char* name, const char* value);
void ExampleInteractive_Run(const struct SolidSyslogMessage* message, FILE* input, ExampleInteractiveSwitchHandler onSwitch);
void ExampleInteractive_Run(const struct SolidSyslogMessage* message, FILE* input, ExampleInteractiveSwitchHandler onSwitch,
ExampleInteractiveSetHandler onSet);
typedef bool (*ExampleInteractiveSetHandler)(void* context, const char* name, const char* value);
void ExampleInteractive_Run(const struct SolidSyslogMessage* message, FILE* input, ExampleInteractiveSwitchHandler onSwitch,
ExampleInteractiveSetHandler onSet, void* context);
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Example/Common/ExampleInteractive.h` around lines 14 - 17, The new set
callback lacks a void* context parameter; update the
ExampleInteractiveSetHandler typedef to accept a void* context as the first (or
last, consistent with ExampleInteractiveSwitchHandler) parameter and add a
matching void* context argument to the ExampleInteractive_Run signature so
callers can pass instance state; ensure ExampleInteractive_Run forwards that
context when invoking onSet (and keep the existing
ExampleInteractiveSwitchHandler/its usage consistent with the shared context
convention).

@DavidCozens DavidCozens May 9, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declined for this slice — leaving as-is.

Rationale: ExampleInteractive lives in Example/Common/, which is Tier 3 (best-effort examples, lowest support bar — see CLAUDE.md "Support tiers"). The Callback Conventions section is in scope for library callbacks (Tier 1/2 Core/, Platform/); the existing ExampleInteractiveSwitchHandler next to this one was added without context for the same reason. Adding context to Set now would also pull Switch into scope per the same section's "the class that owns them is next touched" rule (since this PR touches ExampleInteractive), widening the slice beyond config-injection.

The current FreeRTOS OnSet handler uses file-static globals, which is the documented pattern for the example tier — it's already what the existing POSIX ExampleAppName_Set / ExampleSwitchConfig_SetByName helpers do. There's no current consumer for instance-state, and CLAUDE.md "Don't add features beyond what the task requires" / "Don't design for hypothetical future requirements" applies.

If a multi-instance integrator surfaces a real need, we'll migrate Switch + Set together in one principled change rather than bolting context onto one of them now.

@coderabbitai coderabbitai Bot May 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DavidCozens, understood — the Tier 3 / YAGNI reasoning is clear and well-documented. I'll withdraw the suggestion and record the exemption for future reviews.

✏️ Learnings added 
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 303
File: Example/Common/ExampleInteractive.h:14-17
Timestamp: 2026-05-09T15:56:13.853Z
Learning: In solid-syslog, the CLAUDE.md Callback Conventions section (void* context parameter + paired config field) applies only to Tier 1/2 library callbacks in Core/ and Platform/. Callbacks in Tier 3 example code (Example/Common/, Example/FreeRtos/, etc.) — such as ExampleInteractiveSwitchHandler and ExampleInteractiveSetHandler — are exempt from this requirement. The documented pattern for example-tier handlers is file-static globals. If a multi-instance use-case ever arises, Switch and Set should be migrated together in one principled change rather than piecemeal.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 283
File: Platform/OpenSsl/Interface/SolidSyslogTlsStream.h:7-7
Timestamp: 2026-05-07T08:22:53.110Z
Learning: In solid-syslog, the CLAUDE.md callback rule that requires a `void* context` parameter (and the paired config field) should be applied only to callbacks that need to carry real per-instance state. Exempt callbacks that are stateless thin wrappers over OS primitives—i.e., the callback is effectively a pure function of its input parameters with no instance state to preserve—so adding `void* context` would be pointless (the context would always be NULL). For example, platform implementations like `SolidSyslogSleepFunction` that wrap `nanosleep`/`Sleep` and are defined purely in terms of `int milliseconds` should not add `void* context` or the paired config field. For callbacks that do depend on per-instance data, include `void* context` and the paired config field as required by CLAUDE.md.


EXTERN_C_END

Expand Down
Loading
Loading