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

fix: flawed config handling in the FQBN #2113

Closed
wants to merge 3 commits into from
Closed

fix: flawed config handling in the FQBN #2113

wants to merge 3 commits into from

Conversation

kittaakos
Copy link
Contributor

@kittaakos kittaakos commented Jun 23, 2023

Motivation

Fixes the incorrect config options handling in the FQBN.

Change description

This PR also relaxes the LS assertions at the start. As it turned out, the CLI can detect FQBNs with already appended board config values.

Other information

To set up:

  • Open the 3rd party URLs config.
  • If you have https://www.pjrc.com/teensy/package_teensy_index.json added, remove it.
  • Add https://www.pjrc.com/teensy/package_issue1588_index.json.
  • If you have the Teensy core installed, remove it.
  • Install [email protected]

To verify:

If you had Teensy installed before the verification, you probably know how to revert to the original setup 😊 but in case you don't

  • Remove Teensy @1.57.
  • Replace https://www.pjrc.com/teensy/package_issue1588_index.json with https://www.pjrc.com/teensy/package_teensy_index.json in the 3rd party URLs dialog.
  • Install the latest Teensy version.

Closes #1588

Reviewer checklist

  • PR addresses a single concern.
  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • PR title and description are properly filled.
  • Docs have been added / updated (for bug fixes / features)

@kittaakos kittaakos added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Jun 23, 2023
@kittaakos kittaakos self-assigned this Jun 23, 2023
Also relaxed the LS assertions. As turned out, the CLI can detect FQBNs
with already appended board config values.

Closes #1588

Signed-off-by: Akos Kitta <[email protected]>
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.

UPDATE: fixed by 2473435

An invalid FQBN is generated when compiling for boards that don't have at least one custom board option:
  1. Select File > New Sketch from the Arduino IDE menus.
  2. Select Tools > Board > Arduino AVR Boards > Arduino Uno from the Arduino IDE menus.
  3. Select Sketch > Verify/Compile from the Arduino IDE menus.

🐛 Compilation fails:

Invalid FQBN: invalid config option: 

Compilation error: Invalid FQBN: invalid config option:

With the arduino.cli.daemon.debug setting enabled, I can see the FQBN used in the compile requests is arduino:avr:uno: (note the trailing colon) instead of the correct arduino:avr:uno:

2023-06-27T05:36:00.949Z daemon INFO 49 CALLED: /cc.arduino.cli.commands.v1.ArduinoCoreService/Compile STREAM_RESP
49 |  REQ:  {
49 |    "instance": {
49 |      "id": 1
49 |    },
49 |    "fqbn": "arduino:avr:uno:",
49 |    "sketch_path": "c:\\Users\\per\\AppData\\Local\\Temp\\.arduinoIDE-unsaved2023526-26024-1m7bb86.6w07\\sketch_jun26b",
49 |    "warnings": "none"
49 |  }

2023-06-27T05:36:00.949Z daemon INFO 49 |  ERROR:  rpc error: code = InvalidArgument desc = Invalid FQBN: invalid config option: 
49 STREAM CLOSED

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.

UPDATE: fixed by 0caf0ca

### Describe the problem

Custom board options may have a significant impact on the compilation results so the full FQBN is always passed to Arduino Language Server and it must be restarted whenever the user changes the selection in one of the custom board option menus.

🐛 Changing a custom board option menu selection does not trigger a restart of Arduino Language Server if the pluggable discovery discovery specifies custom board options for the currently selected port.

To reproduce

  1. Start Arduino IDE from the terminal.
  2. Select Tools > Board > Arduino AVR Boards > Arduino Mega or Mega 2560 from the Arduino IDE menus.
    🙂 The terminal logs show something like:
    2023-06-27T05:40:49.229Z root INFO Starting language server: arduino:avr:mega:cpu=atmega2560
    
  3. Select Tools > Processor > ATmega1280Arduino AVR Boards > Arduino Mega or Mega 2560 from the Arduino IDE menus.
    🙂 The terminal logs show that the language server is restarted after the custom board option change. Something like:
    2023-06-27T05:40:58.046Z root INFO Starting language server: arduino:avr:mega:cpu=atmega1280
    
  4. Install the "Teensy" boards platform from https://www.pjrc.com/teensy/package_issue1588_index.json
  5. Connect a Teensy board to your computer with the USB cable.
  6. Select the Teensy board from the board selector in Arduino IDE.
    🙂 The terminal logs show something like:
    2023-06-27T05:42:18.753Z root INFO Starting language server: teensy:avr:teensy41:usb=rawhid,speed=600,opt=o2std,keys=en-us
    
  7. Make a change to one of the custom board options under the Tools menu (e.g., select Tools > Keyboard Layout > Canadian French
    🐛 There is no "Starting language server" log entry in response to the change.

With the arduino.cli.daemon.debug setting enabled, I can see that when I edit the sketch the compile requests that are triggered to build the sketch to update the language server are still using the original FQBN rather than the one that is appropriate for the current menu selections in Arduino IDE.

Expected behavior

The FQBN used with the language server always reflects the current custom board options selections in Arduino IDE.

Arduino IDE version

e47befc (tester build for b0422a7)

Operating system

Windows 11

Akos Kitta added 2 commits June 27, 2023 08:56
FQBNs can be detected with custom board configs. Make sure to sanitize
all FQBNs before comparing them.

Signed-off-by: Akos Kitta <[email protected]>
@kittaakos
Copy link
Contributor Author

Great verification, Per.

49 | "fqbn": "arduino:avr:uno:",

Fixed in 2473435.

7. 🐛 There is no "Starting language server" log entry in response to the change.

Fixed in 0caf0ca.

Please try the tester builds with the most recent changes when you have the capacity. Thank you!

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.

The PR does accomplish the goal of fixing the serious bug #1588 and I am fine if that is considered the entire scope. However, I think it is at least worth mentioning here that I don't think the feature is working as specified in the original implementation request:

arduino/arduino-cli#1660

the IDE could utilize the more specific FQBN to initialize those menu options when a user clicks the port+board from the toolbar's drop-down list.

Describe the problem

Board options are not set according to data from pluggable discovery if the option has previously been set by the user.

To reproduce

Set Up

  1. Install the "Teensy" boards platform from https://www.pjrc.com/teensy/package_issue1588_index.json if not already installed.
  2. Connect a Teensy board to your computer with the USB cable.
  3. Select the Teensy board from the board selector menu in Arduino IDE.
  4. Select Tools > USB Type > Dual Serial from the Arduino IDE menus.
    There is nothing special about this specific option setting. It is chosen arbitrarily to establish a known configuration of the board.
  5. Select Sketch > Upload from the Arduino IDE menus.
  6. Wait for the upload to finish successfully.
  7. Select File > Quit from the Arduino IDE menus.
  8. Rename or delete (:warning: cautiously) the "User Data" folder to clear all saved data about user custom board option menu selections:
    • Windows:
      C:\Users\<user name>\AppData\Roaming\arduino-ide
      
    • Linux:
      ~/.config/arduino-ide
      
    • macOS:
      ~/Library/Application Support/arduino-ide
      

Demo

  1. Start Arduino IDE.
  2. Select the Teensy board from the board selector menu in Arduino IDE.
  3. Open the Tools menu.
    🙂 The "USB Type" menu is set to Dual Serial according to the data from the discovery (following the behavior specified in Provide a way to identify board options via pluggable discovery arduino-cli#1660).
  4. Select Tools > USB Type > Triple Serial from the Arduino IDE menus.
    There is nothing special about this specific option setting. It is chosen arbitrarily to establish a known user selection from the custom board option menu.
    ❗ Do not upload to the board after making this change to the menu. This step is only intended to change the configuration of Arduino IDE, without affecting the data the board will provide to the discovery.
  5. Connect any other Arduino board to your computer.
  6. Select the other Arduino board from the board selector menu in Arduino IDE.
  7. Select File > New Sketch from the Arduino IDE menus.
    This and the previous step were chosen arbitrarily in order to clearly demonstrate that a distinct board+port selection event is occurring in the next step.
  8. Select the Teensy board from the board selector menu in Arduino IDE.
  9. Open the Tools menu.
    🐛 The "USB Type" menu is set to Triple Serial instead of the Dual Serial setting indicated by the data from the discovery (not following the behavior specified in Provide a way to identify board options via pluggable discovery arduino-cli#1660).

Expected behavior

The custom board options menus in Arduino IDE are configured according to the data from the discovery whenever a port is selected that provides board option data.

Arduino IDE version

f4e766a (tester build for 0caf0ca)

Operating system

Windows 11

@kittaakos
Copy link
Contributor Author

I think it is at least worth mentioning here that I don't think the feature is working as specified in the original implementation request:

Thank you! Let's not merge this PR if it's not fully functional.

@kittaakos kittaakos marked this pull request as draft July 3, 2023 07:37
@kittaakos
Copy link
Contributor Author

Those are very helpful instructions. Thank you!

9. instead of the Dual Serial setting indicated by the data from the discovery

Does it mean when the CLI detects a board and board configs are part of the FQBN (in this case, usb=serial2 from the teensy:avr:teensy41:usb=serial2), IDE2 must set those board configs when users select the board from the board selector dropdown? So no matter what board config the user picks from the menus, selecting the board from the dropdown overrules any user config.

@per1234
Copy link
Contributor

per1234 commented Jul 4, 2023

So no matter what board config the user picks from the menus, selecting the board from the dropdown overrules any user config.

I think that behavior is the only way the feature can be useful.

My expected behavior is that at step (9) of the demo instructions I provided in my review that when I open the Tools menu after selecting the board+port from the Board Selector menu, the IDE should have changed the "USB Type" menu selection to "Dual Serial".

The user should then be able to adjust the custom board option menus manually however they like, overriding any default settings that were provided for the selected port by the discovery.


I think there might be some corner cases to consider:

Port changes during upload

In some cases the port may change during the course of an upload (either changing address or disappearing and reappearing), with the IDE following the port change. This automatic port selection should not cause a change in the IDE's custom board option settings according to data from the discovery as it would when the user made that port selection manually.

Manual port selection via the Tools > Port menu

So far we only mentioned the Board Selector menu. I'm feeling ambivalent about whether or not the custom board option menus should be set according to the information from the discovery when a user selects a port via this menu.

I don't see any technical reason why it couldn't be done (yes, it is different from the board selector in that you aren't selecting a board simultaneously, but that could be handled), but I'm currently leaning toward the opinion that the Tools > Port menu should be exclusively a way to select a port, without producing any side effects.

@kittaakos
Copy link
Contributor Author

Closing in favor of #2334 and labeling this as a duplicate.

Please follow and try #2334 instead.

@kittaakos kittaakos closed this Feb 8, 2024
@kittaakos kittaakos added the conclusion: duplicate Has already been submitted label Feb 8, 2024
@kittaakos kittaakos deleted the #1588 branch February 8, 2024 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conclusion: duplicate Has already been submitted topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid FQBN assembled when discovery specifies custom board options
2 participants