Skip to content

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Jun 19, 2024

Please check if the PR fulfills these requirements

See how to contribute

  • 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)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • configuration.schema.json updated if new parameters are added.

What kind of change does this PR introduce?

If a port cannot be identified (i.e. can not be associated with a board) the current version of the CLI requires to specify a board with the -b flag. Previously this wasn't necessary and the monitor would sometimes fail to work because it may require special dtr / rts settings.

This PR now reverts the old behavior that is more convenient but also adds a warning to the user when the board can not be identified and the default settings may not work as expected.

What is the current behavior?

$ arduino-cli board list
Port       Protocol Type        Board Name FQBN Core
/dev/ttyS4 serial   Serial Port Unknown

$ arduino-cli monitor -p /dev/ttyS4
Please specify an FQBN. The board on port /dev/ttyS4 with protocol serial can't be identified
$ arduino-cli monitor -p /dev/ttyS4 -b esp32:esp32:esp32
Connecting to /dev/ttyS4. Press CTRL-C to exit.
^C

What is the new behavior?

$ arduino-cli board list
Port       Protocol Type        Board Name FQBN Core
/dev/ttyS4 serial   Serial Port Unknown

$ arduino-cli monitor -p /dev/ttyS4
Using generic monitor configuration.
WARNING: Your board may require different settings to work!

Monitor port settings:
  baudrate=9600
  bits=8
  dtr=on
  parity=none
  rts=on
  stop_bits=1

Connecting to /dev/ttyS4. Press CTRL-C to exit.
^C
$ arduino-cli monitor -p /dev/ttyS4 -b esp32:esp32:esp32
Using default monitor configuration for board: esp32:esp32:esp32
Monitor port settings:
  baudrate=9600
  bits=8
  dtr=off            <-- notice the esp32 requires DTR=off
  parity=none
  rts=off            <-- and also RTS=off
  stop_bits=1

Connecting to /dev/ttyS4. Press CTRL-C to exit.
^C

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

No

Other information

Fix #2638

@cmaglie cmaglie self-assigned this Jun 19, 2024
@cmaglie cmaglie added type: enhancement Proposed improvement topic: CLI Related to the command line interface labels Jun 19, 2024
Copy link

codecov bot commented Jun 19, 2024

Codecov Report

Attention: Patch coverage is 47.61905% with 22 lines in your changes missing coverage. Please review.

Project coverage is 70.23%. Comparing base (7d00b5b) to head (ca0101f).

Files Patch % Lines
internal/cli/monitor/monitor.go 52.94% 16 Missing ⚠️
internal/cli/arguments/port.go 33.33% 4 Missing ⚠️
internal/cli/arguments/fqbn.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2647      +/-   ##
==========================================
- Coverage   70.26%   70.23%   -0.03%     
==========================================
  Files         222      222              
  Lines       21338    21347       +9     
==========================================
  Hits        14993    14993              
- Misses       5165     5175      +10     
+ Partials     1180     1179       -1     
Flag Coverage Δ
unit 70.23% <47.61%> (-0.03%) ⬇️

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

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

@per1234 per1234 added the topic: code Related to content of the project itself label Jun 20, 2024
@cmaglie cmaglie merged commit 850f22a into arduino:master Jun 24, 2024
@cmaglie cmaglie deleted the monitor_defaults branch June 24, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: CLI Related to the command line interface topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Schema not defined for key 'sketchbook_path' / monitor command now requires FQBN -b ...
3 participants