You asked for that ;-) by litinoveweedle · Pull Request #18 · willglynn/taptap

litinoveweedle · 2025-10-28T13:05:59Z

added build chain
fixed cli args dependencies (port + tcp bug etc.)
fixed Node table Response bug
added reconnect function to serial/tcp source
added TCP keepalive support
added persistence JSON data store (for gateways and nodes info)
added new event to report detected gateways and nodes (persistence event)
changed existing power report event format - BREAKING change, both reports needs to be correlated in the higher layer)
added full fledged CHANGELOG.md (instead of Changes.md)
updated/modified cargo

added arm7

changed arm target to armhf

add GNU build targets

* implemented TCP keep alives * reconnect logic implementation * updated README for new features * read code de-duplication into function

* implemented TCP keep alives * refactored TCP keepalive patch * updated Cargo.lock * fix keepalive retries compilation * compilation fixes * fix unused code * fix arguments comments * updated Help and README * initial reconnect logic implementation * compilation fixes, reformated * updated README for new reconnect feature unified buffer read added reconnect logging * reconnect logic de-duplication into function * implement retry mechanism on connection open fail * README typo fix * improved cli arguments dependencies * arg dependencies

updated Cargo.toml

set reconnect default timeout to 60

* fix invalid NodeTableResponse struct * added infrastructure event in observe mode - to output in JSON gateway and node details (addresses, barcodes, versions etc.) * added persistent storage * Updated README to reflect implementation

* fix invalid NodeTableResponse struct * added infrastructure even in observe mode - to output in JSON gateway and node details (addresses, barcodes, versions etc.) * added persistent storage * Updated README to reflect implementation * Minor logging changes * Release 0.2.0

willglynn

Thanks for the PR!

willglynn · 2025-10-28T13:32:42Z

Cargo.toml

-repository = "https://github.com/willglynn/taptap"
-keywords = ["solar"]
+authors = ["Will Glynn", "LiTinOveWeedle"]
+repository = "https://github.com/litinoveweedle/taptap"


With this PR targeted here, I'd rather:

Suggested change

repository = "https://github.com/litinoveweedle/taptap"

repository = "https://github.com/willglynn/taptap"

First I made PR from main, which was dumb. So I can't easily update PR without breaking my min and I can't do that. If we close this PR here we will loose your comment, so my proposal is to solve those at least in discussion and then you can create your devel branch and I will open new PR from new branch as well.

for the change:
OK, for sure, that was for my fork only.

willglynn · 2025-10-28T13:38:08Z

src/config.rs

+    #[serde(default = "default_keepalive_idle")]
+    pub keepalive_idle: u64,
+    #[serde(default = "default_keepalive_interval")]
+    pub keepalive_interval: u64,
+    #[serde(default = "default_keepalive_count")]
+    pub keepalive_count: u32,


Do these knobs matter? In other words, is there a situation where they need to be turned?

(I haven't encountered any situation where TCP keepalives harmed anything, and if your experience matches, I suggest we just… do keepalives without asking the user.)

I did definitely not observer any wrongdoing, but maybe someone would like to turn those down, as those are set maybe to to often? But even so probably no harm.

So I agree, that code may be removed and we can hardcode TCP keepalive times.

willglynn · 2025-10-28T13:43:08Z

src/gateway/physical/tcp.rs

 }
+
+/// Enable TCP keepalive and configure its parameters as supported by the platform.
+fn enable_keepalive(socket: &TcpStream, cfg: TcpKeepaliveConfig) -> std::io::Result<()> {


I'd like to clean this up, potentially removing config knobs but certainly removing commented out code.

It also occurs to me that we could avoid creating a socket2::Socket via try_clone() (and instead From<TcpStream>) if we used a socket2::Socket everywhere else instead.

Honestly this is over my head, I am afraid you would need to do it yourself. This is my first ever project in Rust and doing it I've already spent all my Github Copilot tokens. ;-)

willglynn · 2025-10-28T13:43:48Z

src/main.rs

+
+        /// Path of the JSON file to provide persistent storage for the infrastructure topology data
+        #[arg(long, required = false, value_name = "FILE", default_value = Some(""))]
+        persistent_file: String,


Should this be Option<String>?

Also, how would you feel about making this state_file: instead? Files are implicitly persistent, while "state" reflects what's in the file.

Yep, shall. Regarding name - could be, but I do already use it in other projects, but probably no big deals, but it can stay...

Hmm I tried that, but Rust reports:

mismatched types Ln 270
expected struct std::string::String
found enum Option<std::string::String>

willglynn · 2025-10-28T13:46:34Z

src/main.rs

+
+    /// The number of times to retry reconnecting before giving up (0 for infinite retries)
+    #[arg(long, required = false, value_name = "INT", default_value = Some("0"))]
+    reconnect_retry: u32,


Suggest either --retry (like curl) or --tries (like wget). Connection failures are the only kind of error with a potentially-finite number of tries, so I don't see an advantage to --reconnect-retry.

There are advantages. One of those is, that taptap will exist if tried unsuccessfully multiple times. That it really nice feature for potential self-healing when used inside other applications (like I do).

Of course! I just mean the name 😄

src/main.rs

willglynn · 2025-10-28T13:59:49Z

src/observer.rs

+        if !file_path.is_file() {
+            log::info!(
+                "persistent file: {} not found, starting with empty state",
+                self.persistent_file
+            );
+            return;
+        }
+        match File::open(&file_path).and_then(|mut file| {


This is a classic TOCTOU bug. The race condition itself isn't an important finding here, but the check is completely avoidable: opening the file may succeed, or it may fail indicating that the file does not exist, or it may fail for some other reason. So, match those outcomes:

match File::open(&file_path) /*…*/ { Ok(data) => { // … }, Err(e) if e.kind() == io::ErrorKind::NotFound => { log::info!( "persistent file: {} not found, starting with empty state", self.persistent_file ); } Err(e) => { // …

Yep, you are completely right. shall be as you propose, but it would be nice to catch also other errors - write permissions etc with some user explaining log messages. But I do not know those errors in Rust - so if you can propose implementation for other error type....

Hmm I checked the code, altough there is potential race condition it would not create any harm, so is not strictly speaking TOCTOU as any file disappearing between the check a opening will be caught in the properly evaluated File::open, so this is kind of looks nicer things...

Ok, got your point - fixed now

willglynn · 2025-10-28T14:44:20Z

src/observer.rs

+        };
+
+        // Write to temporary file
+        let mut file = match File::create(&tmp_path) {


Suggest std::fs::write(&tmp_path, &data) instead.

I think there might be a problem here on Windows (on some filesystems?) due to file still being open while renamed. std::mem::drop(file) or let _ = file would drop it, closing the file, but there's also a write() function which avoids this whole sequence 😄

eeee nope. Write is never atomic operation. While move is (at least on any modern filesystem). We don't want to get our file corrupted. Either it is there completely or it was not updated. So I would propose to keep it. I have 0 windows systems in my household, so I can't tell anything regarding win compatibility, but we can for sure close the file after writing - again I would have to google for how to do that in Rust. ;-)

Agreed! I mean we should std::fs::write() the tempfile, then std::fs::rename() the tempfile over the original.

Actually we shall do:

std::fs::write() std::mem::drop(file) then std::fs::rename()

ok?

std::fs::write() doesn't return the file, so there's nothing to drop. That one call replaces File::create(), File::write(), File::flush() (or maybe it's sync_all()?), and std::mem::drop(file) 😄

It can be just:

std::fs::write(&tmp_path, &data); std::fs::rename(&tmp_path, &file_path);

(with error handling)

OK, I even check rust implementation.

write == write_all, and seems flush is called explicitly... So flush call can be removed as well

#[stable(feature = "rust1", since = "1.0.0")] impl Write for Vec<u8> { #[inline] fn write(&mut self, buf: &[u8]) -> io::Result<usize> { self.extend_from_slice(buf); Ok(buf.len()) } #[inline] fn write_all(&mut self, buf: &[u8]) -> io::Result<()> { self.extend_from_slice(buf); Ok(()) } #[inline] fn flush(&mut self) -> io::Result<()> { Ok(()) } }

willglynn · 2025-10-28T14:46:37Z

src/observer/event.rs

-#[derive(Debug, Copy, Clone, PartialEq, Serialize, Deserialize, JsonSchema)]
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)]
 pub struct PowerReportEvent {
+    pub event_type: String,


Please revert this. PowerReportEvent can only ever be a PowerReportEvent; it shouldn't carry around a String member to that effect.

If you want Event to serialize with a type: "power_report" property, add #[serde(tag = "type")] to Event.

Ok, I understand, but once again it is bit complex for the Rust newbie to serde this and serde this. For sure at least I need to have event_type in the output json to have simple events types tag to parse on. But if this could be done in better way I agree, but please propose exact change. The same can be for the Persistent event JSON>

GitHub doesn't let me add a suggestion to a line that's not changed, but it would be here:

/// An event produced by an observer. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] -#[serde(rename = "snake_case")] +#[serde(tag = "type", rename = "snake_case")] pub enum Event { PowerReport(PowerReportEvent), }

This uses serde's internally tagged enum representation, which seems to be what you're after.

There would be probably similar thing needed to be done to your liking in the 👍
persistent_state.rs

for this code?

#[derive(Debug, Clone, Eq, PartialEq, Serialize, Deserialize)] pub struct PersistentStateEvent { pub event_type: String, pub gateways: BTreeMap<GatewayID, PersistentStateEventGateway>, pub nodes: BTreeMap<GatewayID, BTreeMap<NodeID, PersistentStateEventNode>>, }

I treid to do that, I will test it, lets see.

willglynn · 2025-10-28T14:47:53Z

src/observer/persistent_state.rs

+/// Information like hardware addresses and version numbers are exchanged infrequently. This data
+/// is captured and stored in `PersistentState`.
+#[derive(Debug, Clone, Eq, PartialEq, Default, serde::Serialize, serde::Deserialize)]
+pub struct PersistentState {


Can all this be pub(crate)? I'd rather leave the persistent state as a black box than have open internals which would be covered by eventual semver guarantees.

Same as fr previous comment, I just poorly grasp the Rust struct inheritance. So if you have idea how to improve please share it.

Sure, I'm proposing that everything in this file that's pub might be better as pub(crate). That makes the types, fields, etc. available to everything in taptap without being accessible to other crates which use taptap, and that inaccessibility makes it possible to change how this works without breaking anything outside taptap later.

litinoveweedle · 2025-10-28T15:58:52Z

First I made PR from main, which was dumb. So I can't easily update PR without breaking my min and I can't do that. If we close this PR here we will loose your comment, so my proposal is to solve those at least in discussion and then you can create your devel branch and I will open new PR from new branch as well.

litinoveweedle · 2025-10-28T16:40:22Z

can you please create some dev branch so I can create new PR against it?

willglynn · 2025-10-28T17:03:02Z

Sure, target the new dev branch.

litinoveweedle · 2025-10-28T17:51:58Z

Closing as to be followed in #19

litinoveweedle added 26 commits November 19, 2024 15:56

Create build.yml

ed9bb18

Update build.yml

8ea5990

Create Changes.md

84bf129

Update build.yml

18a24b3

Update build.yml

ae6514c

Merge branch 'willglynn:main' into main

cdc08a5

Update build.yml rust-cross action

8fb614a

Update build.yml

5d967ee

Update build.yml

73109d5

Update main.rs

265d682

Update build.yml

a55ca72

added arm7

Update build.yml

9bc04a3

changed arm target to armhf

add GNU build targets

407e7a2

add GNU build targets

update runner to Ubuntu 22.04

f57c297

Update build.yml

76c3faf

Update build.yml

c06db79

Reconnect logic

2585f51

* implemented TCP keep alives * reconnect logic implementation * updated README for new features * read code de-duplication into function

Fix README typo

30c1cdf

added formatted CHANGELOG.md

901e832

updated Cargo.toml

updated Cargo.lock

afd65ef

bump cargo package release version

0894c44

fix release link to CHANGELOG.md

617ee67

updated README to reflect V0.1.2 chnages

0428723

set reconnect default timeout to 60

willglynn requested changes Oct 28, 2025

View reviewed changes

litinoveweedle mentioned this pull request Oct 28, 2025

Upstream merge #19

Closed

litinoveweedle closed this Oct 28, 2025

	repository = "https://github.com/litinoveweedle/taptap"
	repository = "https://github.com/willglynn/taptap"

Conversation

litinoveweedle commented Oct 28, 2025

Uh oh!

willglynn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

litinoveweedle commented Oct 28, 2025

Uh oh!

litinoveweedle commented Oct 28, 2025

Uh oh!

willglynn commented Oct 28, 2025

Uh oh!

litinoveweedle commented Oct 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone