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

Unify AVR and SAM cores/arduino #5771

Closed
cousteaulecommandant opened this issue Dec 28, 2016 · 43 comments
Closed

Unify AVR and SAM cores/arduino #5771

cousteaulecommandant opened this issue Dec 28, 2016 · 43 comments
Labels
Component: Core Related to the code for the standard Arduino API feature request A request to make an enhancement (not a bug fix)

Comments

@cousteaulecommandant
Copy link
Contributor

Right now hardware/arduino/avr/cores/arduino and hardware/arduino/sam/cores/arduino contain redundant files, for example Print.cpp, WString.cpp, etc. This makes maintenance complicated, since changes on one won't affect the other (even when those changes are platform-independent), so the developer will have to make the changes on multiple files (and something tells me SAM won't have the same attention as AVR). I think it would be a good idea to unify them so that all platform-independent sources are common, and then each derivative has a small set of platform-specific sources.

@dlabun
Copy link

dlabun commented Dec 28, 2016

If you look at Print.cpp on both the AVR and SAM cores you'll see they're different... That's not to say there are not identical files shared between the cores but many of the files are tweaked for their specific core.

@cousteaulecommandant
Copy link
Contributor Author

True, but the differences are minimal. In fact I only found one (that's 1 more than I initially assumed). Still, I think those small differences should be separated as much as possible so that as many files as possible are identical and can be merged. (Maybe this suggestion is not as immediate as I would have imagined)

Also, this separation between high-level functionality and low-level details seems logical from an organization point of view.

@q2dg
Copy link

q2dg commented Dec 28, 2016

something tells me SAM won't have the same attention as AVR

Well, until recent .cc/.org merge, SAM architecture (present only in Due board) was deprecated...

@eric-wieser
Copy link
Contributor

If you look at Print.cpp on both the AVR and SAM cores you'll see they're different

Then put the one different function in PrintFlashString.cpp. C++ does not require all of a class to be in a single file.

@mbanzi
Copy link
Member

mbanzi commented Jan 11, 2017

This is something we're investigating, how to move hw independent files in a different location than the hw dependent files. We'll bring the discussion to the dev list

@q2dg it was never deprecated as a Core, the Arduino Due as a product was marked as retired because we were not going to maintain it further.

@bperrybap
Copy link

Now that the IDE is smarter about the way it handles libraries and can automatically locate sub libraries, it should be possible to make Print its own library and move it to the common bundled libraries area.
The Print class should be core independent.
My guess is that the only reason that certain header files like Print were duplicated in each core was because until recently the IDE was not smart enough to locate sub libraries and the only way to ensure that files like Print.h, Printable.h, Stream.h and a few others were always located was to put them in the core libraries directory. This was because the core libraries directory was always on the include path.
Now that the IDE is smarter, these files could be moved into the bundled libraries area as separate libraries.

@eric-wieser
Copy link
Contributor

@bperrybap: What about Print::print(const __FlashStringHelper *ifsh), which may not even exist on all cores?

@PaulStoffregen
Copy link
Sponsor Contributor

PaulStoffregen commented Jan 18, 2017

The Print class should be core independent.

My Print class implementations for Teensy have hardware-specific optimizations, some even in assembly.

Just because many boards use Arduino's code as-is, please don't assume everyone does.

@bperrybap
Copy link

Which cores don't have F() and the __FlashStringHelper class?
They exist in these cores: IDE bundled AVR, Arduino SAM, teensy AVR, teensy3 ARM, ATTinyCore, ESP8266, STM, chipKit,

But, yeah all the crap to support the proprietary AVR progmem stuff really sucks and has made a wreck of things.
In this case, I don't think including a common Print class as a bundled library wouldl be a problem.
Cores that choose to not provide all the common services (which I would consider the F() macro to be part of standard Arduino services at this point in time), or wish to provide their own can always include their own Print class, which they are already doing today in their core. The Print.h in the core would trump the one in a common libraries area so nothing would break even for those cores that want to provide their own Print class.

However in the case of using _FlashStringHelper class in the common bundled Print class, the common bundled Print class header and code could be made smarter to allow for cores that don't provide the F() service/capability.

IMO, any core that provides an F() macro but does not provide for a __FlashStringHelper class is broken and will have to ship its own Print class vs use the one bundled with the IDE.

To account for F() and __FlashStringHelper class not existing, all that is necessary is to check for the existence of the F macro.
i.e.

#if defined(F)
// put all the code that depends on or uses __FlashStringHelper class in here
#endif

This should work since __FlashStingHelper class should exist whenever F() exists and F should be defined in a core specific file. (currently WString.h)

@PaulStoffregen
Copy link
Sponsor Contributor

PaulStoffregen commented Jan 18, 2017

I believe you are correct on this point about the header file:

The Print.h in the core would trump the one in a common libraries area so nothing would break even for those cores that want to provide their own Print class.

Indeed the group of -I command line used the compile the sketch code has the core library first.

However, the linker command which assembles the final .elf file has core.a last. All the .o files from libraries appear earlier on the linker command. That detail worries me a bit....

You're probably right, but I really hope whatever changes are considered can be throughly tested for "nothing would break even for those cores that want to provide their own Print class".

@dlabun
Copy link

dlabun commented Jan 18, 2017

@bperrybap Did you check the SAMD21 core?

@bperrybap
Copy link

When I said "including a common Print class as a bundled library" I was meaning a separate new bundled library called Print that has a directory under the common libraries directory where Print.h and Print.cpp would live. It would be sitting in the same place with the other bundled libraries like LiquidCrystal. Not a new bundled library that includes other things besides the Print class, like perhaps Printable, or Stream (which, BTW, have similar needs/issues). I'm talking about a very simple change that uses and preserves all the existing IDE build mechanisms.
I believe that there are better ways to handle this kind of stuff - but they are more intrusive, and would probably be more likely to break things.
This was simply a proposal to create a common Print class library that cores could choose to start using, without having to modify the existing build methodology in the IDE and without breaking any existing cores.

But Paul, you bring up a good point about the order of the linked objects and how it has the potential to break things depending on how these common service libraries are implemented.
However, the object ordering issue starts to get very tricky and ugly since the IDE is currently stomping on the users sketchbook libraries area to hold its bundled library updates.
IMO, the IDE should never do this, but it was done this way to avoid doing IDE in place updates.
I believe IDEs should update bundled libraries in place since updates by a given IDE are generally specific to that IDE and should not affect or even break other IDEs that happen to be installed on the same machine. Doing bundled updates in the users sketchbook area is fraught with issues, but I lost that discussion back on the mailing list.
All that said, IDE bundled library updates is for the most part off topic with respect to this topic but it might have some relevance depending on the implementation of these common services; like if multiple of these services (i.e. Print, Printable, Stream) are bundled together into a new library the way the core objects are bundled.

@eric-wieser
Copy link
Contributor

@PaulStoffregen

My Print class implementations for Teensy have hardware-specific optimizations, some even in assembly.

Can you link to these? For the little the print class does, I'm surprised that there's a hardware-specific optimization in there.

@bperrybap
Copy link

@dlabun, I looked at the Arduino.cc samd core, which does include a definition for F() and __FlashStringHelper.
Is that core different than the SAMD21 core you are referring to?
But regardless, even if that core or any other core did something different from all the other cores, if Print used the method I described earlier, it wouldn't matter. It means that any core that doesn't want to use a new bundled Print class wouldn’t have to use it and could continue to use the Print class included in the core.
i.e. a core would have to be modified, by removing its own Print class code in order to use the new bundled Print class code.

@dlabun
Copy link

dlabun commented Jan 19, 2017

No, that's the one I'm referring to... I was just curious if you did look at it.

@eric-wieser
Copy link
Contributor

eric-wieser commented Jan 19, 2017

Proposal - switch to static polymorphism for printing:

  1. Add static-polymorphic printing:

    template<typename T>
    inline int Print::print(const T&& t) {
        return ::print(*this, std::forward<const T>(t));
    }
    // this actually allows all the other println members to be removed
    template<typename T>
    inline int Print::println(const T&& t) {
        int size = print(std::forward<const T>(t));
        return size + println();
    }
  2. Deprecate Printable, and as a stop-gap, define

    int print(Print& p, const Printable& obj) { return p.print(obj); }
  3. Remove all references to __FlashStringHelper and String from Print.h. Define in string.h:

    int print(Print& p, const String& s) { return p.write(s.c_str(), s.length()); }

    And in some core-specific file,

    int print(Print& p, __FlashStringHelper* f) { /* platform specific code */ }

Yes, std::forward is not part of the arduino library. However, as a core part of the c++ language, it seems unreasonable not to include it, along with std::move (#5875)

@bperrybap
Copy link

@dlabun the issue for teensy is that the Print class has extensions that the Arduino team has so far chosen not to embrace so it would still need to include and provide its own Print class.

@PaulStoffregen
Copy link
Sponsor Contributor

Can you link to these?

Sure, why not? Here you go:
https://github.com/PaulStoffregen/cores/blob/master/teensy3/Print.cpp#L143
https://github.com/PaulStoffregen/cores/blob/master/teensy/Print.cpp#L132

For the little the print class does, I'm surprised that there's a hardware-specific optimization in there.

I didn't mean to steer this conversation off-topic. I also really don't have much to say about future versions of Arduino ought to structure this stuff. Really, I'm quite happy with the way it is now. I don't understand why this is an issue, but if people want it another way, the last thing I want to do is stand in the way of progress.

My only concern is to preserve the ability for 3rd party core libs to customize this stuff. I know the Print class may seem very mundane, like nobody would wish to customize it, or if they did, the edits would be minor. As you can see from those links, some of us do indeed publish very different Print class code customized to specific hardware.

@eric-wieser
Copy link
Contributor

I think there are two parts to this - customizing the interface, and customizing the implementation. Clearly you've got a good argument for wanting to do the latter. However, allowing the former to happen can result in code written for one version of Print to fail for another.

@PaulStoffregen
Copy link
Sponsor Contributor

PaulStoffregen commented Jan 19, 2017

Or from an alternate point of view, I guess it could be said some of us might be a bit Obsessive Compulsive about optimizations....

@PaulStoffregen
Copy link
Sponsor Contributor

Actually, one of the many low-priority items on my todo list involves an interface change to print 64 bit double and 64 bit integers. Obviously that would make no sense on AVR where double precision floats don't even exist. Please don't close the door to those ideas by rigidly fixing the interface.

@eric-wieser
Copy link
Contributor

eric-wieser commented Jan 19, 2017

@PaulStoffregen: I think my comment here allows you to do that without needing to modify Print.h and Print.cpp. You'd just add a function in your code-specific code that is

int print(Print& p, int64_t i) {
    // do stuff with the low-level parts of p to print this
}

@PaulStoffregen
Copy link
Sponsor Contributor

I must confess, I don't normally use these C++ features, so for now I'm going to just take your word for it.

@cousteaulecommandant
Copy link
Contributor Author

I think that it's better to just make the small fragment of platform-dependent code separate from the rest, kind of like what #5775 proposes.

For the specific case of __FlashStringHelper, I think the best would be to have a "fetch byte from flash string" function/macro that works on all platforms. This could be called pgm_read_byte(), which is already defined for AVR, so it'd simply be a matter of adding this to SAM (on a "SAM-specific definitions" header):

#define PGM_P const char *
#define pgm_read_byte(address_short) (*(address_short))

(this is similar to how it's done in AVR, but using a * operator to fetch the byte rather than __LPM(); see http://www.nongnu.org/avr-libc/user-manual/group__avr__pgmspace.html)

PS: @eric-wieser: I also used templates for println in #5829, because who wants to have Print.cpp that bloated?

@oqibidipo
Copy link

@cousteaulecommandant Haven't you noticed that the SAM (and SAMD) core already has
<avr/pgmspace.h> emulation?

@cousteaulecommandant
Copy link
Contributor Author

cousteaulecommandant commented Jan 19, 2017

@cousteaulecommandant Haven't you noticed that the SAM (and SAMD) core already has
<avr/pgmspace.h> emulation?

...well, it is obvious I hadn't, wasn't it? 😁

OK, so this means that both versions of Print.cpp could be identical (except maybe for efficiency reasons, but I don't think they'll really affect this). I'd go that way: make as many files identical when possible (unless there's a good reason not to do so) and put platform-dependent low-level functionality on separate files.

@cousteaulecommandant
Copy link
Contributor Author

@PaulStoffregen

My Print class implementations for Teensy have hardware-specific optimizations, some even in assembly.

I think a good solution could be doing something like

// start of file: sam/cores/arduino/Print.cpp
#include "Print.h"
#include "avr/cores/arduino/Print.cpp"
//   end of file: sam/cores/arduino/Print.cpp

i.e. make all duplicate files just a "symbolic link" to the original one. If a third party platform needs to reuse all the code, then it can leave this #include; otherwise it'll have to actually copy all the content and do the relevant modifications. (This may be inconvenient for platforms that only need to modify a fragment of a file though, but if the high/low functionality is correctly split into files this should't be a frequent problem.)

@bperrybap
Copy link

@cousteaulecommandant
I have no idea what you are meaning by "symbolic link".
It seems much simpler to just move Print.h/Print.cpp files to become its own Print library in the bundled libraries area and if a core does not want to use it, it can simply supply its own to override it.
The only thing that needs to be clarified is the specific types & functions used to handle any progmem functions by the bundled Print library, which for the most all the existing 3rd party cores are already using the same types/names for this so it becomes a matter of documenting what is already being done.

@cousteaulecommandant
Copy link
Contributor Author

I meant that having a Print.cpp file that just #includes a different Print.cpp file is pretty much like having a "mirror" of that other Print.cpp file, as changes in the latter will reflect directly in the former. I guess making a library would also work, as long as it's possible to replace some functionality when needed.

@PaulStoffregen
Copy link
Sponsor Contributor

PaulStoffregen commented Jan 19, 2017

Use of an include with relative pathname might be complicated when you factor in the many ways the Arduino IDE currently supports installing core libraries. They can be folders within "hardware", or folders within "portable", or folders within "~/.arduino15/packages" on Linux, or folders within "~/Library/Arduino15" on Mac, or folders within "%LocalAppData%/Arduino15" on Windows (which may be a very different pathname on XP & Vista... or if system customization changes the location of %AppData% or %LocalAppData%), or some other location that's not quite clear to me when using the new Windows Store version. Some of these use a subdirectory structure with version numbers, which might further complicate efforts to embed relative pathnames.

Edit: The new sandbox capability of Mac Sierra should also be kept in mind. My understanding is Arduino doesn't use it. I'm not 100% up to speed on the latest Sierra changes, but I do believe they make relative pathnames from within the application bundle to anything outside rather problematic, particularly the user & system Library folders. Apparently Mac malware has exploited such things recently, so Apple is likely to further enforce app bundle best practices in the not-too-distant future.

@cousteaulecommandant
Copy link
Contributor Author

I was not thinking on using the actual file path. My idea was to handle this via -I, not via relative paths. This is, when compiling for SAM, the flags would be something like -Ipath/to/sam -Ipath/to/avr in such a way that SAM files can #include "foo.h" for the SAM version, or #include "avr/foo.h" or #include "common/foo.h" for the "base version".

@cmaglie
Copy link
Member

cmaglie commented Jan 24, 2017

@bperrybap
I tried to move the Print class in a library, I moved the three files Print.h, Print.cpp and Printable.h but the compilation fails with:

"/home/cmaglie/Code/arduino/build/linux/work/hardware/tools/avr/bin/avr-g++" -c -g -Os
 -Wall -Wextra -std=gnu++11 -fpermissive -fno-exceptions -ffunction-sections -fdata-sections
 -fno-threadsafe-statics -MMD -flto -mmcu=atmega32u4 -DF_CPU=16000000L
 -DARDUINO=10802 -DARDUINO_AVR_LEONARDO -DARDUINO_ARCH_AVR
 -DUSB_VID=0x2341 -DUSB_PID=0x8036 '-DUSB_MANUFACTURER="Unknown"'
 '-DUSB_PRODUCT="Arduino Leonardo"'
 "-I/home/cmaglie/Code/arduino/build/linux/work/hardware/arduino/avr/cores/arduino"
 "-I/home/cmaglie/Code/arduino/build/linux/work/hardware/arduino/avr/variants/leonardo"
 "/home/cmaglie/Code/arduino/build/linux/work/hardware/arduino/avr/cores/arduino/CDC.cpp"
 -o "/tmp/arduino_build_698416/core/CDC.cpp.o"
In file included from /home/cmaglie/Code/arduino/build/linux/work/hardware/arduino/avr/cores/arduino/HardwareSerial.h:29:0,
                 from /home/cmaglie/Code/arduino/build/linux/work/hardware/arduino/avr/cores/arduino/Arduino.h:232,
                 from /home/cmaglie/Code/arduino/build/linux/work/hardware/arduino/avr/cores/arduino/USBAPI.h:33,
                 from /home/cmaglie/Code/arduino/build/linux/work/hardware/arduino/avr/cores/arduino/CDC.cpp:19:
/home/cmaglie/Code/arduino/build/linux/work/hardware/arduino/avr/cores/arduino/Stream.h:26:19: fatal error: Print.h: No such file or directory
 #include "Print.h"

compilation terminated.
Uso la libreria Print nella cartella: /home/cmaglie/Code/arduino/build/linux/work/hardware/arduino/avr/libraries/Print (legacy)
exit status 1
Errore durante la compilazione per la scheda Arduino Leonardo.

the Print library is detected but is not used when compiling the core, in particular the core is build without any "external" include path. To allow your proposal we should change the build process and add the include path of all the detected libraries in the build command for the core, I'm not sure how much this change may impact and if it is fully backward compatible.

@cousteaulecommandant

I was not thinking on using the actual file path. My idea was to handle this via -I, not via relative paths. This is, when compiling for SAM, the flags would be something like -Ipath/to/sam -Ipath/to/avr in such a way that SAM files can #include "foo.h" for the SAM version, or #include "avr/foo.h" or #include "common/foo.h" for the "base version".

that's an interesting idea, but why you would allow to include AVR stuff from SAM core? There are a lot of architectures availalble through the boards manager (samd, arc32, stm, pic... just to name a few of them) following your suggestion we should make all of them cross-accessible once installed and that doesn't look like a good idea to me.

For the record, I ingenuously tried to introduce a similar approach for libraries, some years ago, the idea was similar to yours (adding an arch folder with a subfolder for each architecture arch/avr, arch/sam...etc.., while keeping the arch-independent code inside src). It didn't worked out well and generated a lot of polemics and concerns, IMHO not for the proposal per-se (that I still think it was good) but because core-maintainers wants to keep control on their code, following their own pace and independence.

That said, I think that it's good to provide a "core API" but at the same time we must allow maintainers to make change/additions at their wishes, even allow them use an old version of the API if needed (I'm thinking about cores that are no more maintained or in very low maintenance level).

I'd like to propose/try a slightly different approach: put all the common files into a CoreAPI repostiory and let the maintainers copy them in a subfolder of the core, say cores/arduino/api/.... This may sound a bit inefficient but has the advantage that each core is self-contained and allows the maintainers to update the core API at their own pace and eventually apply patches etc. (of course the copy is for distribution only, during developement you can use git submodule or symbolic links or any other trick that simplify integration from the CoreAPI repository).

@cousteaulecommandant
Copy link
Contributor Author

why you would allow to include AVR stuff from SAM core?

Only because AVR is "the base core" all other cores "inherit" from; my idea was that any non-AVR arch would be "like AVR but...". Maybe it's better to separate "avr" and "common" though, even if only AVR is supported at the end.

I think copying the core API would be a good idea; that way if core Arduino changes something it's up to the maintainer to update their architecture or not.

@bperrybap
Copy link

@cmaglie
It sounds like the IDE doesn't use the libraries & their paths it detected for the core library compiles. I do agree that there might be some subtle backward compatibility issues. But in reality it seems like more of a behavior change that might break some users (or core developers) expectations of how they are using some currently somewhat nebulously defined behaviours. One potential stumbling block might be the library hierarchy process of library selection. One potential issue may relate to the decision to not do IDE bundled updates in place (which I still think is a bad idea). I can't remember exactly how all the library hierarchy works right now, but having the IDE install its bundled library updates into the user's sketchbook/libraries area (which IMO should not be used for this purpose) is very problematic. And since the user's sketchbook/libraries area should trump all other libraries, that means that an IDE library update of bundled library will not only stomp on top of a user's personal version of library and override all other installed IDE versions version of the library (like it does now) but may also trump a core that ships its own version of that library.
Many, if not all of these library hierarchy and clobbering library issues go away if the IDE were to do in place library updates for bundled libraries instead of using the users sketchbook/libraries area.

In terms of having this "core API" library concept where library code is shared by copying it around, I don't think that is a good idea. That essentially already exists today. Today, each core has its own copy of all these "core API" library files in their core and can modify them as they see fit. So, if I understood this "core API" library concept correctly, it seems to be just providing a repo for them and then moving the files from one location to another within the core - which creates additional work for the core maintainers - without solving anything. And cores that do not wish to deviate from the "official" core API library won't be able to use and get updates to the official files without having to modify and re-ship their core since they must be copied to reside within the core.
In general, I think the idea of sharing common code through copying is a bad idea. The better concept is to have a way to use/reuse the very same single/common copy of code and not create all these copies, and then, if desired (which it seems there is a desire), also have a mechanism that allows overriding the common version of the code.

I would suggest that any solution, have a way for the the IDE to bundle these common API classes, so that by default cores can use them but then provide a way for cores or users to override them.
It would seem that the library mechanism should be an ideal way of doing that as that is what it was designed to do and already provides a way to ship bundled libraries and allow cores and users to override a bundled library.
That way, the IDE can ship common/official versions of API class libraries, and cores that do not wish to take on the maintenance of providing a modified version of those of API class files, don't have to do anything and will automatically get any updates to them should a future IDE ship with updates to them. But at the same time cores like say Teensy that want to ship a custom version of a common API class library are free to do so.

So my preference would be to use a library mechanism, and if there are currently some issues with using the existing library mechanism, fix the library mechanism so that it can be used for this. This may mean revisiting the way the IDE does its bundled updates.
But in the end, it just seems like the answer is to have a good library mechanism that can be used to share a single copy of common code vs creating new mechanisms to copy around common code.

@cmaglie
Copy link
Member

cmaglie commented Jan 24, 2017

One potential stumbling block might be the library hierarchy process of library selection.
[...]
One potential issue may relate to the decision to not do IDE bundled updates in place (which I still think is a bad idea).

In this specific issue the hierarchy is not the problem, if you look at the log in my previous comment the Print library has been correctly selected. About in-place update, we cannot do them, most O.S. won't allow that. Also disabling the update at all is not ideal, because the libraries should be updatable independently from the IDE. As I said many times, the bundled libraries are a legacy (error?) of the past and (unless there is a good reason) I don't want to create new.

BTW this discussion about the bundled libraries is totally offtopic, I don't understand why you keep on pushing it again and again, so let's go back in topic 😉 :

That way, the IDE can ship common/official versions of API class libraries, and cores that do not wish to take on the maintenance of providing a modified version of those of API class files, don't have to do anything and will automatically get any updates to them should a future IDE ship with updates to them. But at the same time cores like say Teensy that want to ship a custom version of a common API class library are free to do so.

The devil is in the details, can you fork Arduino and show me how you would do it? I'm doing it to "test" my proposal.

It would be much more convincing than 50 pages of text.

In terms of having this "core API" library concept where library code is shared by copying it around, I don't think that is a good idea. That essentially already exists today. Today, each core has its own copy of all these "core API" library files in their core and can modify them as they see fit. So, if I understood this "core API" library concept correctly, it seems to be just providing a repo for them and then moving the files from one location to another within the core - which creates additional work for the core maintainers - without solving anything. And cores that do not wish to deviate from the "official" core API library won't be able to use and get updates to the official files without having to modify and re-ship their core since they must be copied to reside within the core.

The difference is that the Core API currently do not formally exists. Each core made a best-effort implementation of what is currently considered the official API. Look at the current AVR core for example, are you able tear apart the API from the specific implementation?

Moreover the Arduino API is not only made by Print, but by all the classes that should be architecture independent so Stream, String, IPAddress, Server, Client, etc.etc. those classes are more or less bound together, if you update one separately you may break something else. Also most of the content of Arduino.h like HIGH LOW, some functions like digitalWrite or attachInterrupt etc, may be considered as "common", how would you handle that?

I'm not saying that the "put everything in a library" approach is wrong, just to consider all the details in the process.

In general, I think the idea of sharing common code through copying is a bad idea. The better concept is to have a way to use/reuse the very same single/common copy of code and not create all these copies, and then, if desired (which it seems there is a desire), also have a mechanism that allows overriding the common version of the code.

Factoring the "common" part in a specific folder will make a net distinction on what is the API and what is the implementation. As I said, copying is only for distribution while the developers can use git submodules or symbolic links, or whatever trick their favourite OS provide to keep track of upstream changes.

@bperrybap
Copy link

I do understand the value of having a "common API" code area.
Things do get complicated pretty quickly when looking at all the common code as separate libraries because there are so many interdependencies. As an example, many classes depend on Print so there becomes an issue of how to keep everything in sync if all the parts are independently up-datable. And Print is just one small example of the many common API/code interdependencies.
There is no easy solution or magic bullet- which may steer towards using a more/larger grouped approach which is where I think you are leaning.

However, the biggest concern I have with the proposal, is that it doesn't seem to actually ship the common files/classes with the IDE in a way that allows 3rd party cores to simply use them without having to include/copy them into their core. It seems to be requiring that all 3rd party core developers copy them into their cores, including cores that do not modify them. This means that 3rd party core maintainers will have to do a new core release any time they change, including those that are not doing any deviation from them.
Other than the cleaner/separate location might make it easier to see, track, merge the changes/updates from arduino.cc (which is a definite big plus), in the broader picture, it does not seem to be much different from what exists today in that it still requires 3rd party core maintainers to copy & duplicate lots of arduino.cc code that they don't modify into their core.
So from the larger picture, while I think this proposal has the potential to make it easier for core developers to keep their common API code in their cores in sync with the official arduino.cc common API code, it doesn't seem to offer a way ensure they are automatically in sync for cores that don’t deviate from the arduino.cc code which I think was the essence of the OP's original concern.

=======================================================================
In terms of moving the common API code to new bundled libraries:

Obviously, the compile issue you saw when moving the files to be a new bundled library called Print is not related to in place bundled library updates. And yes, there are many classes/files beyond the Print class that are architecture independent that should not be lumped in with the core specific files.
I was looking at issues much further ahead than that simple issue that relate to using the library mechanism for common/shared code classes/files and potential issues related to library manager updates.
I understand what you have done for this simple test (moved Print.cpp, Print.h, and Stream.h to a bundled "Print" library) and I understand why it is not working. And clearly the bundled library update issues are not the cause of why modules like CDC.cpp or HardwareSerial.cpp or others will not compile.
But the use and discussion of in place library updates by the library manager and how it relates to library hierarchy is definitely not off topic.
Looking beyond the current simple issue of compilation, there are issues with respect to handling bundled library updates when using the existing bundled library mechanism for common/shared code as to what can potentially happen when doing updates on bundled libraries that are common/shared code.
For example, if the existing IDE library mechanism is used and common API classes like Print.cpp & Print.h were moved to their own bundled library called Print, then, since the IDE library manager uses/steals the user's sketchbook/libraries for holding its bundled library updates, if an update to the new Print bundled library were to be done and if the users sketchbook/libraries area is a higher priority than a core library (which it should be), then the IDE's bundled library updates end up clobbering core versions of the library. And even if that specific issue is resolved, the bundled update for the Print library overrides ALL Print classes for all the other IDEs that are installed which can potentially break other IDE versions installed. And even more of an issue, at least for the time being, is that since the cores currently don't use the common libraries headers or code, any core that shipped its own Print class would use its Print code for its core file compiles but the sketch would be built using the user's sketchbook/libraries versions of the Print code - which may be different from what is in the core.
This is exposing some of the issues related to the way the library manager does its updates that have the potential to break updates for common API code.
And THAT is why it is on topic.
It is that next level of detail of trying to use multiple bundled libraries for common/shared code that I'm referring to and thinking about.
Any common/shared code mechanism needs to work not only for the initial installation of the IDE and core but also not should not break should the IDE library manager allow and do a bundled common code library update.
In my view, the existing bundled library update process used by the library manager is already so full of issues that it shouldn't be used. And if the common/API class code were to use the existing library mechanism by moving it to be bundled libraries, then when the IDE library manager does a bundled update for common/API code, it can create issues for the common/API code just like it does today with all the existing bundled libraries.

Depending on if/how the common API code is divided up into bundled libraries, the way the IDE include paths and the way library manager does bundled library updates, has the potential to break 3rd party cores when an updated for a bundled common/API library is done.

So again, that is why how library the manager does bundled library updates is on topic.

@per1234
Copy link
Collaborator

per1234 commented Jul 27, 2018

@cousteaulecommandant
Copy link
Contributor Author

Chainsaw is indeed related, although it's not the "common files / arch-specific files" idea I had in mind for the Arduino core library.
Having separate projects for the IDE and the core code was something that was really necessary, and it makes sense that each architecture has its own project.
However, this might complicate bug fixes / new features affecting all architectures since there isn't an ArduinoCore-common project.
Then again, having an ArduinoCore-common project with all other projects depending on it would probably be a nightmare, and not necessary if patches can be easily exported from one project to another (which is probably the case) -- that way, new features and common bug fixes would be developed in the main ArduinoCore project and then ported to other projects as needed.

If exporting patches to other ArduinoCore-* projects is easy, I think this issue could be closed.
(In any case, the motivation for this issue was that having to port a patch to all architectures was tedious; Chainsaw at least allows you to only do it for a single architecture, so this issue has already been half-addressed.)

@matthijskooijman
Copy link
Collaborator

@cousteaulecommandant, that is exactly what Chainsaw is about, to cut out the common parts of all cores and put them into a shared repository (or some other similar implementation, that allows code to be shared between all cores, and updating each with minimal effort).

@sandeepmistry
Copy link
Contributor

The core API can be found here: https://github.com/arduino/ArduinoCore-API

However, older cores (avr, sam, samd) have yet to be updated to use it.

@matthijskooijman
Copy link
Collaborator

I am going to go ahead and close this issue already, since the AVR and SAM cores are no longer in this repository, and the effort to fix this is underway (and will happen, regardless of this issue being open). If further discussion is needed, an issue can be opened in the API repository, or the AVR and/or SAM repositories.

@bperrybap
Copy link

Is there an issue in the API repository to track this? as it seems premature to close out an issue before the in-progress effort to resolve it is completed.
Also seems like it would be useful to have an issue there that points back to this issue to maintain historical perspective and to track things until it is fully resolved.

@matthijskooijman
Copy link
Collaborator

@bperrybap fair point. I guess that an issue in each separate repo would be the most logical, but a single issue to track all of these is probably more useful, so I made arduino/ArduinoCore-API#95.

@per1234 per1234 added Component: Core Related to the code for the standard Arduino API feature request A request to make an enhancement (not a bug fix) labels May 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Core Related to the code for the standard Arduino API feature request A request to make an enhancement (not a bug fix)
Projects
None yet
Development

No branches or pull requests