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

Added --timestamp option to monitor command. #2336

Merged
merged 3 commits into from
Sep 25, 2023

Conversation

zvonler
Copy link
Contributor

@zvonler zvonler commented Sep 22, 2023

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$ 

@CLAassistant
Copy link

CLAassistant commented Sep 22, 2023

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Sep 22, 2023

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.

@alessio-perugini alessio-perugini added the topic: code Related to content of the project itself label Sep 22, 2023
@alessio-perugini
Copy link
Contributor

alessio-perugini commented Sep 22, 2023

👋 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. 💪

@zvonler
Copy link
Contributor Author

zvonler commented Sep 22, 2023

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

@zvonler
Copy link
Contributor Author

zvonler commented Sep 22, 2023

Test added and formatting fixed.

@zvonler
Copy link
Contributor Author

zvonler commented Sep 22, 2023

Oops, fixed that last formatting issue.

@alessio-perugini
Copy link
Contributor

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

image

@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
93 checks passed
@alessio-perugini alessio-perugini added this to the Arduino CLI 0.35.0 milestone Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
3 participants