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

Board autoswitch #7349

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Board autoswitch #7349

wants to merge 6 commits into from

Conversation

timkoers
Copy link

@timkoers timkoers commented Mar 20, 2018

Merged the auto board selector code into the master branch.

Also the BareMinimum.ino has got the new header right now

See arduino/arduino-ide#2438

@facchinm
Copy link
Member

@ArduinoBot build this please

@ArduinoBot
Copy link
Contributor

❌ Build failed.

@facchinm
Copy link
Member

Error is

[javac] .../arduino-core/src/processing/app/Sketch.java:168: error: cannot find symbol
    [javac]   private void removeBuildSettingsHeader(EditorTab tab){
    [javac]                                          ^
    [javac]   symbol:   class EditorTab
    [javac]   location: class Sketch
    [javac] .../arduino-core/src/processing/app/Sketch.java:235: error: cannot find symbol
    [javac]   public void setBuildSettings(EditorTab tab, LinkedHashMap<String, String> buildSettings){
    [javac]                                ^
    [javac]   symbol:   class EditorTab
    [javac]   location: class Sketch

Probably missing an include that Eclipse resolved on its own?

This should fix the build error
@facchinm facchinm added the Component: IDE user interface The Arduino IDE's user interface label Mar 20, 2018
@facchinm
Copy link
Member

Still no luck... to make sure it builds on out CI servers, run ant build from inside the build folder until it doesn't complain anymore 😉

@arduino arduino deleted a comment from ArduinoBot Mar 21, 2018
@facchinm
Copy link
Member

@ArduinoBot build this please

@timkoers
Copy link
Author

timkoers commented May 4, 2018

Is this going to be merged?

Copy link

@lmihalkovic lmihalkovic left a comment

Choose a reason for hiding this comment

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

Hmm ... 👎🏻
(‘Node32s’ .. among other things)

@timkoers
Copy link
Author

timkoers commented Jun 1, 2018

@lmihalkovic what do you mean with 'Node32s'?

@matthijskooijman
Copy link
Collaborator

(not responding to previous comments, just reviewing this change)

Overall, this seems like a great addition. Storing this inside the sketch is probably a good idea too, as it is probably very transparent for users. One downside is when using version control for a sketch, switching the board changes the file, but that is only minor.

However, I'm not quite convinced of the current format chosen. Using a block comment with a header like /** Arduino IDE Board Tool details seems way too fragile to me. It's not very recognizable as a special token, and it is easy to accidentally mess up. The fact that the block is automatically added and managed helps, though. A simpler approach might be to not use a block, but just line comments, like // ARDUINO-OPTIONS: board=arduino:avr:uno (though I'm not quite sure what the right, sufficiently generic header would be).

I haven't looked at the implementation in detail, but there does seem to be room for improvement. In particular, I'm a bit puzzled by this line, which I think means that this feature only works to distinguish between multiple variants of the Node32s board, which is obviously not something that is generic enough to be merged. I suspect this is also what @lmihalkovic referred to.

Some other thoughts:

  • Now, this handles board selection, but if we ever implement per-sketch build options / defines, that should likely use the same mechansim (but the format is already parsed to generic key-value pairs it seems).
  • How is this handled wrt multiple .ino files? Are all of them scanned? Just the primary one?
  • How does/would this interact with external editor mode?
  • As discussed in Allow configuring board for IDE to select by default when sketch is opened arduino-ide#2438, some of this code (in particular parsing and updating the sketch code) might need to be moved to a codebase that is shared between Create and the Java IDE (e.g. like arduino-builder).
  • Should the preprocessor be run before checking the options block? For storing the default board selection, this is probably not needed, but when later adding build options, it could be very powerful to set build options based on the e.g. the IDE version, or library versions. OTOH, this induces a circular dependency: changing the build options or board type based on this special comment block means different preprocessor defines, so you would need to run the preprocessor again, which could change the build options again, etc. Just doing a few runs until the resulting options are stable could perhaps work.

@lmihalkovic
Copy link

lmihalkovic commented Jun 1, 2018

@timkoers IMHO there are many ways to address this need. The Editor/EditorTab/Sketch/Controller code is ... well .. lets just say that i had do do ‘some’ cleanup in order to create a number of extensions that should be easy to do, but were not, due to the structure of the aforementioned code. Going into details would take time that i’d rather spend doing something else, as it would in the end be a matter of two conflicting views of what constitutes ‘good code’.
All in all, I think this is a clever hack more than a engineering solution. Looking at what atolic or vstudio do might provide a good starting point. (I have nothing against hacks.. if they do not contribute to making the next innovation harder than it could be)

PS: it is tiresome to have to stroke egos.. the code editor/../.. code is just poorly organised and the most recent round of changes did not improve the situation (textstorage or whatever the name was is a good idea done wrong). This code does the same: adds more enthropy to someonthing that has already plenty as it is

@briandking
Copy link

Maybe a workable solution would be to store a copy of preferences.txt (PREFS_FILE) inside the project directory?
It looks like the main preferences loading first reads a global default, and then overlays a user specific preferences.txt (https://github.com/arduino/Arduino/blob/master/arduino-core/src/processing/app/PreferencesData.java#L55), so wouldn't it be possible on opening a project, check for preferences.txt in the project directory, and overlay that in the same way as the user specific preferences override the defaults? That would:

  • keep the preferences outside of the code, but applying to a whole project so you wouldn't end up with file specific preferences like the comment block proposal.
  • keep the preferences within the project so revision management software (like git) would also track the settings
  • re-use an already defined format for storing these preferences
  • possibly allow re-use of the code that already reads and parses these preferences.txt files
  • allow for per projects settings that are not only board related (eg. you could have per project recent files, per project window positioning, per project font and language settings, and per project boardsmanager.additional.urls

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: IDE user interface The Arduino IDE's user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants