Skip to content
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.

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

Allow rewinding of commands sent via UART buffer #1345

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

dymk
Copy link
Collaborator

@dymk dymk commented Oct 1, 2024

Allows for flow control like repeat to be used via the serial buffer. The lookback itself is limited to a fixed buffer size, but is useful for simple loops such as repeating a few gcode commands.

This was so I could add more comprehensive tests to the flow control logic before any refactors. Tests an edge case for repeat with negative values (casts anything <= 0 to "no repeats").

Tested with a gcode fixture, fixture_tests/fixtures/flow_control_repeat.nc

@MitchBradley
Copy link
Collaborator

Could the functionality be implemented with std::queue instead of a new circular buffer class?

@MitchBradley
Copy link
Collaborator

Or perhaps std::deque, which is fast at both ends.

@MitchBradley
Copy link
Collaborator

It is possible that deque might cost more RAM than the circular buffer. It would be good to find out. RAM is in short supply given the FluidNC feature creep that has been occurring.

@dymk
Copy link
Collaborator Author

dymk commented Oct 1, 2024

Thanks for taking a look - indeed, both std::queue and std::deque will allocate on each push (or every N pushes), as they're implemented internally with linked lists of elems or chunks of elems. The motivation for a new circular buffer class was to do a single heap allocation on start, and never allocate again after that.

@dymk
Copy link
Collaborator Author

dymk commented Oct 1, 2024

In terms of overhead, there would be at least one additional pointer for each element for std::queue, and two for std::dequeue. So for the Buf<char> case, we'd have at least a 4 or 8 bytes per stored element. std::vector will only be around two std::size_t of fixed overhead.

@MitchBradley
Copy link
Collaborator

Okay I'm convinced. I wasn't sure that deque was necessarily done with linked lists. That is of course an obvious way, but sometimes C++ libs have clever optimizations for degenerate cases.

@dymk dymk merged commit 1edaac6 into dymk/refactor-run-fixtures Oct 1, 2024
24 checks passed
@dymk
Copy link
Collaborator Author

dymk commented Oct 1, 2024

It's up to the underlying implementation; std::deque looks like it tends to allocate space for ~8 elements at a time, so that lowers the overhead. But that would mean an allocation every 8 chars, and I worry memory fragmentation would eventually bite us.

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