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

Add debug output pane to cu-consolemon #219

Merged
merged 6 commits into from
Jan 18, 2025

Conversation

AS1100K
Copy link
Contributor

@AS1100K AS1100K commented Jan 13, 2025

Fixes #153

Demo

Screen.Recording.2025-01-17.224847.mp4

@AS1100K AS1100K changed the title Add debug and stdout/stderr output pane to cu-consolemon WIP Add debug and stdout/stderr output pane to cu-consolemon Jan 13, 2025
@AS1100K AS1100K marked this pull request as draft January 13, 2025 15:08
Copy link
Collaborator

@gbin gbin left a comment

Choose a reason for hiding this comment

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

Thanks, this is a good start.

components/monitors/cu_consolemon/src/lib.rs Outdated Show resolved Hide resolved
components/monitors/cu_consolemon/src/lib.rs Show resolved Hide resolved
@AS1100K AS1100K force-pushed the add-debug-output branch 4 times, most recently from 217ec7f to 7ded214 Compare January 15, 2025 12:56
@AS1100K AS1100K changed the title WIP Add debug and stdout/stderr output pane to cu-consolemon Add debug output pane to cu-consolemon Jan 15, 2025
@AS1100K AS1100K marked this pull request as ready for review January 15, 2025 13:00
@AS1100K
Copy link
Contributor Author

AS1100K commented Jan 16, 2025

@makeecat Thanks for the review, I have now used VecDeque and DashMap. I have also, migrated from using Mutex to Channel. I did this, as when using Mutex there are a lot of instances of long blocking, which makes the application feel laggy. Using Channel blocking has been prevented and thus also improves performance as now the single thread read and writes the log without attaining any lock.

Also, I think we should make this Debug pane behind feature gate as this library is using tracing set_global_default which is not meant to be used by libraries as if the main crate maybe wants to send these logs or store them somewhere else but then they will not be able to use monitor.

I thinking adding a default feature like debug-pane would be good and will introduce flexibility for the main crate user on log behaviour.

@makeecat
Copy link
Collaborator

makeecat commented Jan 16, 2025

I agree with adding a debug-panel faeture gate. In the future we may want to use tokio async to handle all IO-related operations to avoid overhead, but we can start with this implementation at the current stage.

@makeecat makeecat requested a review from gbin January 16, 2025 14:21
components/monitors/cu_consolemon/src/lib.rs Outdated Show resolved Hide resolved
components/monitors/cu_consolemon/src/lib.rs Outdated Show resolved Hide resolved
components/monitors/cu_consolemon/Cargo.toml Outdated Show resolved Hide resolved
components/monitors/cu_consolemon/src/lib.rs Outdated Show resolved Hide resolved
@gbin
Copy link
Collaborator

gbin commented Jan 16, 2025

Thanks @AS1100K! My biggest question here is why do we want to introduce tracing in the middle of it. Also, I don't see where we keep the buffer from not growing indefinitely, we should have somewhere a constant or parameter saying: we keep the last 1000 lines of back buffer. Otherwise your robot will run out of memory at some point :)

@AS1100K
Copy link
Contributor Author

AS1100K commented Jan 16, 2025

Also, I don't see where we keep the buffer from not growing indefinitely

I also kept that in mind, and only keep the number of logs according to terminal height. here https://github.com/copper-project/copper-rs/pull/219/files#diff-77cbd66ea06743fe2a1e13a87d5525ab87330d6cc6d548f35068b256168b611cR352-R356

@gbin
Copy link
Collaborator

gbin commented Jan 16, 2025

Also, I don't see where we keep the buffer from not growing indefinitely

I also kept that in mind, and only keep the number of logs according to terminal height. here https://github.com/copper-project/copper-rs/pull/219/files#diff-77cbd66ea06743fe2a1e13a87d5525ab87330d6cc6d548f35068b256168b611cR352-R356

aaaah... ok ... we need a back buffer :) people will need to scroll back up a little bit

@AS1100K AS1100K force-pushed the add-debug-output branch 4 times, most recently from bc3a73f to 4dbf360 Compare January 17, 2025 17:53
@AS1100K
Copy link
Contributor Author

AS1100K commented Jan 17, 2025

@gbin Thanks for you review, I have now removed the support for tracing crate and now it supports cu29-log and log crate. Also, while doing so I introduced a new argument in LoggerRuntime::init function. I would love some feedback here as I am not sure if this is a right call here.

Also, I don't see where we keep the buffer from not growing indefinitely

I also kept that in mind, and only keep the number of logs according to terminal height. here https://github.com/copper-project/copper-rs/pull/219/files#diff-77cbd66ea06743fe2a1e13a87d5525ab87330d6cc6d548f35068b256168b611cR352-R356

aaaah... ok ... we need a back buffer :) people will need to scroll back up a little bit

Also, I would also implement it soon

@gbin
Copy link
Collaborator

gbin commented Jan 17, 2025

Cool! Thank you for your patience!

cu-consolemon is not part of core, I don't think it should: for example in production robots should not have it and might use a different implementation behind the Monitoring API.

what about hooking that up either as a Log destination (that will be picked up when consolemon is in) or, another possible way is to add a logging API to the monitoring API (that consolemon relies on and decouples it from core)?

Copy link
Collaborator

@gbin gbin left a comment

Choose a reason for hiding this comment

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

Ok, 2 comments left and we merge this.

components/monitors/cu_consolemon/src/lib.rs Outdated Show resolved Hide resolved
core/cu29_log_runtime/Cargo.toml Outdated Show resolved Hide resolved
@gbin
Copy link
Collaborator

gbin commented Jan 18, 2025

Thanks @AS1100K good work!

@gbin gbin merged commit 94e54bc into copper-project:master Jan 18, 2025
7 checks passed
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.

Provide a new pane with the debug and stdout/stderr output for cu-consolemon
3 participants