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.

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

Jump to bottom

RSDK-5787 - implement installer option to print monitor logs to file #130

Merged
merged 2 commits into from
Dec 19, 2023
Merged

RSDK-5787 - implement installer option to print monitor logs to file #130

merged 2 commits into from
Dec 19, 2023

Conversation

gvaradarajan
Copy link
Member

corresponding commit on fork of espflash here: viamrobotics/espflash@8f8e58b

@gvaradarajan gvaradarajan requested a review from a team as a code owner December 7, 2023 20:10
@gvaradarajan gvaradarajan force-pushed the RSDK-5787-monitor-logs-file-option branch from 06de90f to 16d6f21 Compare December 7, 2023 20:11
npmenard
npmenard approved these changes Dec 11, 2023
View reviewed changes
Copy link
Member

@npmenard npmenard left a comment

Choose a reason for hiding this comment

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

LGTM. Right not we are truncating the content of the logfile which I think we should avoid. Also could be add a separator line with date and time so we can track roughly when the logs where generated?

@gvaradarajan gvaradarajan force-pushed the RSDK-5787-monitor-logs-file-option branch from 16d6f21 to a1f5d97 Compare December 18, 2023 20:28
@gvaradarajan
Copy link
Member Author

Note: A follow-up commit on our espflash fork was required to address @npmenard 's requested changes

npmenard
npmenard approved these changes Dec 19, 2023
View reviewed changes
Copy link
Member

@npmenard npmenard left a comment

Choose a reason for hiding this comment

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

LGTM

@gvaradarajan gvaradarajan merged commit 1441558 into viamrobotics:main Dec 19, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@npmenard npmenard npmenard approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gvaradarajan @npmenard