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

Get default port address and protocol from sketch profile using monitor -s <sketchPath> #2329

Merged
merged 7 commits into from
Oct 24, 2023

Conversation

MatteoPologruto
Copy link
Contributor

@MatteoPologruto MatteoPologruto commented Sep 20, 2023

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?

Code enhancement

What is the current behavior?

It is not possible to retrieve port address and protocol information from a sketch profile. monitor cannot run if -p <port> is not specified.

What is the new behavior?

Default port address and protocol can be retrieved from the sketch profile using the -s <sketchPath> flag. -p <port> and -l <protocol> have priority over the information stored in the profile.

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

No

Other information

@MatteoPologruto MatteoPologruto added type: enhancement Proposed improvement topic: code Related to content of the project itself labels Sep 20, 2023
@MatteoPologruto MatteoPologruto self-assigned this Sep 20, 2023
@CLAassistant
Copy link

CLAassistant commented Sep 20, 2023

CLA assistant check
All committers have signed the CLA.

@MatteoPologruto MatteoPologruto force-pushed the monitor-profile branch 2 times, most recently from 9a33f35 to 710cee4 Compare September 20, 2023 09:46
@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (d8694ec) 64.06% compared to head (6c0a0f3) 64.48%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2329      +/-   ##
==========================================
+ Coverage   64.06%   64.48%   +0.41%     
==========================================
  Files         207      207              
  Lines       19546    19593      +47     
==========================================
+ Hits        12523    12634     +111     
+ Misses       5927     5867      -60     
+ Partials     1096     1092       -4     
Flag Coverage Δ
unit 64.48% <90.41%> (+0.41%) ⬆️

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

Files Coverage Δ
internal/cli/arguments/port.go 60.67% <100.00%> (+15.84%) ⬆️
internal/cli/arguments/sketch.go 80.00% <100.00%> (+1.42%) ⬆️
internal/cli/compile/compile.go 73.17% <100.00%> (ø)
internal/cli/debug/debug.go 73.48% <100.00%> (ø)
internal/cli/upload/upload.go 56.73% <100.00%> (ø)
internal/cli/board/attach.go 25.42% <0.00%> (ø)
internal/cli/monitor/monitor.go 68.75% <90.32%> (+5.97%) ⬆️

... and 6 files with indirect coverage changes

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

@cmaglie
Copy link
Member

cmaglie commented Sep 29, 2023

This won't solve the issue request #2280:

$ arduino-cli board list
Port         Protocol Type              Board Name           FQBN                    Core
/dev/ttyACM0 serial   Serial Port (USB) Arduino MKR GSM 1400 arduino:samd:mkrgsm1400 arduino:samd

$ arduino-cli board attach -b arduino:samd:mkrgsm1400 -p /dev/ttyACM0
Default port set to: /dev/ttyACM0
Default FQBN set to: arduino:samd:mkrgsm1400

$ cat sketch.yaml 
default_fqbn: arduino:samd:mkrgsm1400
default_port: /dev/ttyACM0
$ arduino-cli compile
Sketch uses 12448 bytes (4%) of program storage space. Maximum is 262144 bytes.
Global variables use 2996 bytes (9%) of dynamic memory, leaving 29772 bytes for local variables. Maximum is 32768 bytes.

Used platform Version Path
arduino:samd  1.8.13  /home/megabug/.arduino15/packages/arduino/hardware/samd/1.8.13
$ arduino-cli upload
Atmel SMART device 0x10010005 found
Device       : ATSAMD21G18A
Chip ID      : 10010005
Version      : v2.0 [Arduino:XYZ] Oct  4 2017 08:53:16
Address      : 8192
Pages        : 3968
Page Size    : 64 bytes
Total Size   : 248KB
Planes       : 1
Lock Regions : 16
Locked       : none
Security     : false
Boot Flash   : true
BOD          : true
BOR          : true
Arduino      : FAST_CHIP_ERASE
Arduino      : FAST_MULTI_PAGE_WRITE
Arduino      : CAN_CHECKSUM_MEMORY_BUFFER
Erase flash
done in 0.834 seconds

Write 12448 bytes to flash (195 pages)
[==============================] 100% (195/195 pages)
done in 0.074 seconds

Verify 12448 bytes of flash with checksum.
Verify successful
done in 0.011 seconds
CPU reset.
New upload port: /dev/ttyACM0 (serial)
$ arduino-cli monitor
Error getting port settings details: No monitor available for the port protocol default

The expected output of arduino-cli monitor is to connect to the default port specified in the sketch.yaml.
Moreover:

  • The check for the presence of the --port flag has been removed (because it could be obtained cia sketch.yaml). Still, it must be checked anyway later to see if the port has been obtained in one way or another and print a meaningful message if the port is still missing, now we get the very unclear: Error getting port settings details: No monitor available for the port protocol default.
  • The compile and upload commands syntax have the sketch specified as the first non-flag argument:
    arduino-cli compile -b arduino:avr:uno /home/user/Arduino/MySketch
    arduino-cli upload /home/user/Arduino/MySketch
    
    IMHO it would be better to follow the same semantic in the monitor command. I know that it sounds a bit weird to specify a sketch for the monitor command, but that's how it is :-). As a bonus, you can reuse the same code from compile and upload to determine the sketch path.

@alessio-perugini alessio-perugini force-pushed the monitor-profile branch 2 times, most recently from c0704e7 to f45c9fc Compare October 18, 2023 18:26
@alessio-perugini alessio-perugini self-assigned this Oct 18, 2023
@alessio-perugini alessio-perugini force-pushed the monitor-profile branch 2 times, most recently from ffc5920 to 049861a Compare October 19, 2023 09:15
Copy link
Member

@cmaglie cmaglie left a comment

Choose a reason for hiding this comment

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

👍🏼

@alessio-perugini alessio-perugini merged commit ba72833 into arduino:master Oct 24, 2023
95 of 96 checks passed
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The monitor command does not recognize port specified in the sketch.yaml file.
5 participants