Description
While reviewing #10816 (recently used boards), I had a closer look at the custom board menu code and I believe there's a few things about that code that are fishy and fragile, and would deserve improvement. These are a few separate, but heavily related issues, so I'm listing them in detail below.
TL;DR: I would propose to:
- Not translate menu titles
- Do not preserve board options across board changes
- Allow remembering board options for boards selected earlier only through the in-development recently-used-boards list
- Simplify the plethora of current-board-related preferences and use a single
fqbn
property instead
One overall remark (that I made previously here), is that maybe most of this code can be replaced by code that just lets arduino-cli
figure out various options and menus available somewhere in the near future. If this code is refactored now, that option should certainly be considered as well, so any work done now can be somewhat smoothly migrated to that later.
Menu titles are translated
The titles of board menus, as defined in boards.txt
, are translated. This makes some sense for Arduino-supplied cores, where Arduino can both control the boards.txt
entries and the translations in the IDE, but for third-party boards, which have no control over the translation, this will result in untranslated entries, mixed entries (some translated, some untranslated) or even incorrect translations (when the board menu use a word that is also used in the IDE, but in a different context).
I would suggest just not translating these titles, just like we did with platform names before (see #9238 and e003049).
See:
Arduino/app/src/processing/app/Base.java
Line 1596 in a056a5f
Arduino/app/src/processing/app/Base.java
Line 1472 in a056a5f
Comparing titles to look for options
Board menu JMenuItems are created in one place and then looked up in another place. This uses the menu title against the JMenuItem label to find the right one, which is fragile. If two different options within the same core would use the same title (which makes no real sense, but could happen), then options of both would be mixed in the same menu I think. Better to use the id of the menu instead (by storing the id as a client property, or maybe use an (id,platform)-to-menu-item map rather than searching a long list).
See:
Arduino/app/src/processing/app/Base.java
Line 1596 in a056a5f
Arduino/app/src/processing/app/Base.java
Line 1668 in a056a5f
Arduino/app/src/processing/app/Base.java
Line 1472 in a056a5f
Boards array only contains one value ever?
In this bit of code, a boards
array is created, possibly with the intent of collecting all boards that share a given option? However, AFAICS, this always results in an array with just one item every, since subAction
is always a new object, so subAction.getValue("board");
will always return null
?
Arduino/app/src/processing/app/Base.java
Lines 1602 to 1613 in a056a5f
In practice, only the first element of boards
is used anyway, it seems:
Arduino/app/src/processing/app/Base.java
Line 1604 in a056a5f
Saving custom values to preferences.txt is fragile and leads to seemingly arbitrary saving and forgetting of custom options
Currently, custom menu options are stored like custom_optionname=boardid_optionvalue
. I suppose that including the boardid in the value is intended to prevent preserving values from one board to another or something? Or maybe the boards
array mentioned above was originally intended to canonicalize this board id to the first boardid within a platform, to allow preserving options between boards in a platform but not outside? In any case, AFAIU the current behavior is that:
- When you select some options for one board, then switch to another board, then any shared options will be overwritten with values for the new board.
- When you come back to the original board, the overwritten options will not match the board id, so be reset to the defaults again and overwritten. However, any options that were not overwritten (because the other board does not support them), will be kept, so their old values are used.
- In effect, this means that while switching between boards, coming back to an earlier board, some options will be preserved and some will not, rather arbitrarily.
Also note that the boardid
used in the custom_...
options in preferences is the unqalified board id, so if two platforms contain a board by the same name, then switching between them will preserve options, which makes no sense to me.
See also:
Arduino/app/src/processing/app/Base.java
Line 1604 in a056a5f
Using _
as a separator is fragile, since board ids might also contain underscores. However,, it seems that when loading these preferences, only very primitive board id matching is used, not even checking the _
after the board id:
In practice, this means that a custom option stored for one board, e.g. custom_opt=myboardv3_foo
would be applied to another board as well, but in a broken way (e.g. for the myboard
board, this would apply the value 3_foo
).
To fix this, I would consider just not preserving options at all between different boards and also not when coming back to an earlier board. IOW, whenever you select a different board, just reset all options to the default values.
For options which are shared between a lot of different boards, saving options could be a useful addition (does not work right now), but in practice most of these options are really just core options (debug level, flash layout, etc.) or upload options (upload speed) and should probably be treated as such (i.e. see arduino/arduino-cli#922).
One exception could be that for the recently-used-boards list (as being developed in #10816), one would probably expect to get the same options as the last time this particular board was used.
To implement this, and at the same time heavily simplify implementation, I would suggest removing the current board
, target_package
, targe_platform
and custom_*
preferences, and replace them with a single fqbn
preference (as already used by arduino-builder and arduino-cli). The recently-used-boards list can then also just store a list of fqbn's and easily include all board options in there.
The code is complex and slow
The creation of these board menus is quite complex. IIUC, whenever the boards menu is rebuilt, all possible board menus for all possible platforms are created with all possible options for all boards, most of which will be invisible most of the time. Loading preferences happens by remembering items to click later, which is not elegant and probably leads to running some things repeatedly (such as this call). This slowness was already discussed in #10214 before.
I haven't looked closely yet, but maybe just all of this code should be replaced by something that is a lot simpler and just regenerates the custom menu options whenever the current board changes?