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

Extended Pluggable Monitor specification to allow board-specific settings to be applied #1855

Merged
merged 6 commits into from
Sep 2, 2022

Conversation

cmaglie
Copy link
Member

@cmaglie cmaglie commented Aug 30, 2022

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)
  • UPGRADING.md has been updated with a migration guide (for breaking changes)
  • What kind of change does this PR introduce?
    Adds the possibility for board developers to override the default port settings when the monitor is opened.
    This is useful in particular for the serial port settings of dtr and rts since the Arduino IDE 1.8.x allowed the following board.txt properties:

    BOARD_ID.serial.disableRTS=true
    BOARD_ID.serial.disableDTR=true
    

    To generalize the above to the Pluggable Monitor a new way to override specific port settings has been added:

    BOARD_ID.monitor_port.PROTOCOL.SETTING_NAME=SETTING_VALUE
    

    where:

    • BOARD_ID is the board identifier
    • PROTOCOL is the port protocol
    • SETTING_NAME and SETTING_VALUE are the port setting and the desired value

    this is general enough to cover also the old use case (dtr and rts).
    For backward compatibility we will automatically convert the old directives disableRTS and disableDTR to the new one.

  • What is the current behavior?
    The directives:
    BOARD_ID.serial.disableRTS=true
    BOARD_ID.serial.disableDTR=true
    
    are ignored.
  • What is the new behavior?
    The directives above are followed.

@codecov
Copy link

codecov bot commented Aug 30, 2022

Codecov Report

Merging #1855 (a2621d6) into master (05d1446) will increase coverage by 0.06%.
The diff coverage is 43.63%.

@@            Coverage Diff             @@
##           master    #1855      +/-   ##
==========================================
+ Coverage   36.39%   36.46%   +0.06%     
==========================================
  Files         231      231              
  Lines       19573    19618      +45     
==========================================
+ Hits         7124     7154      +30     
- Misses      11620    11635      +15     
  Partials      829      829              
Flag Coverage Δ
unit 36.46% <43.63%> (+0.06%) ⬆️

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

Impacted Files Coverage Δ
arduino/cores/board.go 88.76% <0.00%> (-2.05%) ⬇️
commands/monitor/monitor.go 0.00% <0.00%> (ø)
commands/monitor/settings.go 0.00% <0.00%> (ø)
arduino/cores/packagemanager/loader.go 73.17% <100.00%> (+1.37%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cmaglie cmaglie requested a review from per1234 August 31, 2022 10:31
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

Describe the problem

Pluggable monitor port configuration properties are not recognized when set via custom board options

To reproduce

Create a board definition that sets pluggable monitor port configuration properties via custom board options:

menu.rts=RTS
menu.dtr=DTR
foo.name=Configurable
foo.menu.rts.on=On
foo.menu.rts.on.monitor_port.serial.rts=on
foo.menu.rts.off=Off
foo.menu.rts.off.monitor_port.serial.rts=off
foo.menu.dtr.on=On
foo.menu.dtr.on.monitor_port.serial.dtr=on
foo.menu.dtr.off=Off
foo.menu.dtr.off.monitor_port.serial.dtr=off

Set the option via the --fqbn argument of an arduino-cli monitor command.

🐛 The port is not configured by the property.

Expected behavior

Pluggable monitor port configuration properties can be set via custom board options.

If this is not going to be supported, then the deficiency must be documented in the Arduino Platform Specification.

Arduino CLI version

6499950

Operating system

Windows 10

Additional context

Arduino IDE 1.x recognizes the properties as expected when set via custom board options:

menu.rts=RTS
menu.dtr=DTR
bar.menu.rts.on=On
bar.menu.rts.on.serial.disableRTS=false
bar.menu.rts.off=Off
bar.menu.rts.off.serial.disableRTS=true
bar.menu.dtr.on=On
bar.menu.dtr.on.serial.disableDTR=false
bar.menu.dtr.off=Off
bar.menu.dtr.off.serial.disableDTR=true

@cmaglie
Copy link
Member Author

cmaglie commented Sep 1, 2022

Pluggable monitor port configuration properties are not recognized when set via custom board options

I rebased the PR and fixed this issue, now the submenu options are supported.
This helped also to fix a weird edge case.

@cmaglie cmaglie requested a review from per1234 September 1, 2022 12:21
@per1234 per1234 added the topic: code Related to content of the project itself label Sep 1, 2022
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

Thanks Cristian!

@cmaglie cmaglie merged commit 25d9100 into arduino:master Sep 2, 2022
@cmaglie cmaglie deleted the monitor_config branch September 2, 2022 09:23
@per1234 per1234 added type: enhancement Proposed improvement type: imperfection Perceived defect in any part of project labels Sep 3, 2022
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 type: enhancement Proposed improvement type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants