Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Added --timestamp option to monitor command. #2336

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

Merged
alessio-perugini merged 3 commits into arduino:master from zvonler:monitor_timestamp
Sep 25, 2023

Conversation

Copy link
Contributor

@zvonler zvonler commented Sep 22, 2023 •
edited
Loading

Please check if the PR fulfills these requirements

  • The PR has no duplicates (please search among the Pull Requests
    before creating one)
  • The PR follows
    our contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce?

New feature in the monitor that can add a timestamp to the beginning of each received line.

What is the current behavior?

No such option.

What is the new behavior?

When --timestamp is provided, the monitor wraps the output tty with a timestamping Writer. The format of the timestamps is YYYY-mm-dd HH:MM:SS.

Does this PR introduce a breaking change, and is titled accordingly?

Negative.

Other information

This is my first time writing Go (except for the sample program I used to work out the approach), so please review carefully.

There don't appear to be any docs specifically about the monitor command, nor tests of the CLI entry point.

The change was tested by hand using this sketch:

void setup()
{
 Serial.begin(115200);
}
static const uint32_t period = 100; // millis
void loop()
{
 static uint32_t last_print_tm = 0;
 static uint8_t next_ch = 0;
 static const char* message = "tick\n";
 if (millis() - last_print_tm > period) {
 Serial.print(message[next_ch++]);
 if (message[next_ch] == '0円')
 next_ch = 0;
 last_print_tm += period;
 }
 delay(10);
}

Testing confirmed that individual non-newline characters received are still output immediately, and only newline characters cause the timestamp output:

Zacharys-iMac:arduino-cli zvonler$ ./arduino-cli -b adafruit:avr:metro monitor -p /dev/cu.usbserial-015F62D6 -c baudrate=115200 --quiet
tick
tick
tick
tick
ti^C
Zacharys-iMac:arduino-cli zvonler$ ./arduino-cli -b adafruit:avr:metro monitor -p /dev/cu.usbserial-015F62D6 -c baudrate=115200 --quiet --timestamp
[2023年09月22日 12:12:18] tick
[2023年09月22日 12:12:19] tick
[2023年09月22日 12:12:19] tick
[2023年09月22日 12:12:20] tick
[2023年09月22日 12:12:20] tic^C
Zacharys-iMac:arduino-cli zvonler$

umbynos reacted with heart emoji
Copy link

CLAassistant commented Sep 22, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

Copy link

codecov bot commented Sep 22, 2023 •
edited
Loading

Codecov Report

Patch coverage: 67.64% and project coverage change: +0.01% 🎉

Comparison is base (0e6133f) 63.03% compared to head (a664c2d) 63.04%.
Report is 2 commits behind head on master.

Additional details and impacted files 
@@ Coverage Diff @@
## master #2336 +/- ##
==========================================
+ Coverage 63.03% 63.04% +0.01% 
==========================================
 Files 201 201 
 Lines 19264 19286 +22 
==========================================
+ Hits 12143 12159 +16 
- Misses 6067 6072 +5 
- Partials 1054 1055 +1
Flag Coverage Δ
unit 63.04% <67.64%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
internal/cli/monitor/monitor.go 22.90% <67.64%> (+7.21%) ⬆️

... and 26 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

alessio-perugini commented Sep 22, 2023 •
edited
Loading

👋 Thank you, for the contribution. The implementation looks good. 🤩
As you noticed, we do not have any integration tests for the monitor as we cannot provide a real board in the CI.
We can do a unit test in the same folder you're working on by creating a new monitor_test.go file. You can look at this for some inspiration. In this case, the package name doesn't need to have the suffix _test as we need to access a private function.

The idea is to test only the timeStampWriter. These are some hints:

  • We call the newTimeStampWriter providing something that satisfies the io.Writer like (bytes.Buffer{})
  • We "simulate" the data coming from the board by passing arbitrary input in the timeStampWriter.Write func
  • Once done writing, we should be able to assert the content of the buffer if has the timestamp as a prefix.

Don't forget to run go fmt ./... from the root of the project. Or if you have task installed you can run task go:format. (That should solve the CI error)

If you need help don't hesitate to ping me I'd be more than happy to guide you. 💪

Copy link
Contributor Author

zvonler commented Sep 22, 2023

Thanks for the pointers, I'll add a test like you described.

Copy link
Contributor Author

zvonler commented Sep 22, 2023

Test added and formatting fixed.

Copy link
Contributor Author

zvonler commented Sep 22, 2023

Oops, fixed that last formatting issue.

Copy link
Contributor

I've done some manual testing, and it works flawlessly. Thank you for your contribution and your patience. 🤓

image

zvonler reacted with thumbs up emoji umbynos reacted with heart emoji

@alessio-perugini alessio-perugini linked an issue Sep 25, 2023 that may be closed by this pull request
3 tasks
@alessio-perugini alessio-perugini changed the title (削除) Added --timestamp option to monitor command to fix #2316. (削除ここまで) (追記) Added --timestamp option to monitor command. (追記ここまで) Sep 25, 2023
@alessio-perugini alessio-perugini merged commit 6ebfb1d into arduino:master Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@alessio-perugini alessio-perugini alessio-perugini approved these changes

Reviewers whose approvals may not affect merge requirements
Labels
topic: code Related to content of the project itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Monitor: show timestamp when receiving messages

AltStyle によって変換されたページ (->オリジナル) /