-
-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Comments
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. |
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. |
Well, until recent .cc/.org merge, SAM architecture (present only in Due board) was deprecated... |
Then put the one different function in |
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. |
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. |
@bperrybap: What about |
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. |
Which cores don't have F() and the __FlashStringHelper class? But, yeah all the crap to support the proprietary AVR progmem stuff really sucks and has made a wreck of things. 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.
This should work since __FlashStingHelper class should exist whenever F() exists and F should be defined in a core specific file. (currently WString.h) |
I believe you are correct on this point about the header file:
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". |
@bperrybap Did you check the SAMD21 core? |
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. 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. |
Can you link to these? For the little the print class does, I'm surprised that there's a hardware-specific optimization in there. |
@dlabun, I looked at the Arduino.cc samd core, which does include a definition for F() and __FlashStringHelper. |
No, that's the one I'm referring to... I was just curious if you did look at it. |
Proposal - switch to static polymorphism for printing:
Yes, |
@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. |
Sure, why not? Here you go:
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. |
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 |
Or from an alternate point of view, I guess it could be said some of us might be a bit Obsessive Compulsive about optimizations.... |
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. |
@PaulStoffregen: I think my comment here allows you to do that without needing to modify int print(Print& p, int64_t i) {
// do stuff with the low-level parts of p to print this
} |
I must confess, I don't normally use these C++ features, so for now I'm going to just take your word for it. |
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 #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 PS: @eric-wieser: I also used templates for |
@cousteaulecommandant Haven't you noticed that the SAM (and SAMD) core already has |
...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. |
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.) |
@cousteaulecommandant |
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. |
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. |
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 |
@bperrybap
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.
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 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 |
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. |
@cmaglie 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. 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. 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. |
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 😉 :
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.
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 I'm not saying that the "put everything in a library" approach is wrong, just to consider all the details in the process.
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. |
I do understand the value of having a "common API" code area. 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. ======================================================================= 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. 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. |
Related to "Project Chainsaw": |
Chainsaw is indeed related, although it's not the "common files / arch-specific files" idea I had in mind for the Arduino core library. If exporting patches to other ArduinoCore-* projects is easy, I think this issue could be closed. |
@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). |
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. |
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. |
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. |
@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. |
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.
The text was updated successfully, but these errors were encountered: