Open Flole998 opened 3 years ago
Memory usage change @ 21d2bf19c2e26b6e2e3cc07ef148658db3636ba2
Board | flash | % | RAM for global variables | % |
---|---|---|---|---|
arduino:avr:LilyPadUSB | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:atmegang:cpu=atmega168 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:atmegang:cpu=atmega8 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:bt:cpu=atmega168 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:bt:cpu=atmega328 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:chiwawa | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:circuitplay32u4cat | 0 - 0 | 0.0 - 0.0 | 0 - 0 | N/A |
arduino:avr:diecimila:cpu=atmega168 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:diecimila:cpu=atmega328 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:esplora | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:ethernet | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:fio | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:gemma | 0 - 0 | 0.0 - 0.0 | 0 - 0 | N/A |
arduino:avr:leonardo | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:leonardoeth | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:lilypad:cpu=atmega168 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:lilypad:cpu=atmega328 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:mega:cpu=atmega1280 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:mega:cpu=atmega2560 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:megaADK | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:micro | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:mini:cpu=atmega168 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:mini:cpu=atmega328 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:nano:cpu=atmega168 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:nano:cpu=atmega328 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:nano:cpu=atmega328old | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:one | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:pro:cpu=16MHzatmega168 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:pro:cpu=16MHzatmega328 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:pro:cpu=8MHzatmega168 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:pro:cpu=8MHzatmega328 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:robotControl | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:robotMotor | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:uno | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:unowifi | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:yun | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:yunmini | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
If the buffer length is going to be made configurable, it might be a good idea to take the opportunity to rename the define to something less likely to clash with third-part code. "BUFFER_LENGTH" is very, very generic. (ARDUINO_WIRE_BUFFER_LENGTH? I don't know if there's an Arduino coding standard for this.)
Absolutely agree with above. Great idea to have buffer configurable - ARDUINO_WIRE_BUFFER_LENGTH
I would like to see this feature in the official ArduinoCore!
I have personally used seperate receiving and sending buffer lengths in the past, so I submitted changes to @per1234's original fork, so that they can be included in this PR after @per1234 accepts my pull request in their fork.
I also included the suggestions from @obra and @neilh10 to make sure that the define is rather unique and unlikely to conflict with existing defines.
I submitted changes to @per1234's original fork, so that they can be included in this PR after @per1234 accepts my pull request in their fork.
Hi @Wendelstein7. I think you mentioned the wrong GitHub user in your comment. Although I do happen to have a fork of this repository at the moment, it is only used to stage an unrelated PR for a trivial change to the keywords.txt
file
Sorry, if this is not the right place to ask this, but do you, in general, accept pull requests for ArduinoCore from users on github? Is it the right way to go, if one would want to contribute?
Cheers,
On Wed, Jan 12, 2022 at 2:56 AM per1234 @.***> wrote:
I submitted changes to @per1234 https://github.com/per1234's original fork, so that they can be included in this PR after @per1234 https://github.com/per1234 accepts my pull request in their fork.
Hi @Wendelstein7 https://github.com/Wendelstein7. I think you mentioned the wrong GitHub user in your comment. Although I do happen to have a fork of this repository at the moment, it is only used to stage an unrelated PR for a trivial change to the keywords.txt file
— Reply to this email directly, view it on GitHub https://github.com/arduino/ArduinoCore-avr/pull/430#issuecomment-1010458461, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABLN7LP572A43TRKHNQFTFTUVS4DBANCNFSM5EDOBRAQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>
Apparently I was the one who was supposed to be mentioned ;) I've merged the PR, thanks!
Memory usage change @ 8fd20884361d33e97d9db1a30f37c2e0c8287659
Board | flash | % | RAM for global variables | % |
---|---|---|---|---|
arduino:avr:LilyPadUSB | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:atmegang:cpu=atmega168 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:atmegang:cpu=atmega8 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:bt:cpu=atmega168 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:bt:cpu=atmega328 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:chiwawa | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:circuitplay32u4cat | 0 - 0 | 0.0 - 0.0 | 0 - 0 | N/A |
arduino:avr:diecimila:cpu=atmega168 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:diecimila:cpu=atmega328 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:esplora | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:ethernet | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:fio | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:gemma | 0 - 0 | 0.0 - 0.0 | 0 - 0 | N/A |
arduino:avr:leonardo | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:leonardoeth | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:lilypad:cpu=atmega168 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:lilypad:cpu=atmega328 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:mega:cpu=atmega1280 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:mega:cpu=atmega2560 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:megaADK | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:micro | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:mini:cpu=atmega168 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:mini:cpu=atmega328 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:nano:cpu=atmega168 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:nano:cpu=atmega328 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:nano:cpu=atmega328old | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:one | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:pro:cpu=16MHzatmega168 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:pro:cpu=16MHzatmega328 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:pro:cpu=8MHzatmega168 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:pro:cpu=8MHzatmega328 | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:robotControl | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:robotMotor | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:uno | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:unomini | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:unowifi | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:yun | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
arduino:avr:yunmini | 0 - 0 | 0.0 - 0.0 | 0 - 0 | 0.0 - 0.0 |
Hi @Wendelstein7. All contributors to a PR are required to sign Arduino's Contributor License Agreement (CLA) before it can be merged. You are now a contributor in this PR, so you will need to sign it, as @Flole998 has already done (thanks!).
Please sign the CLA by clicking the "Contributor License Agreement" link in the comment by the @[]()CLAassistant bot above: https://github.com/arduino/ArduinoCore-avr/pull/430#issuecomment-920433087
I accepted the Contributor License Agreement. Everything should be good to go! Sorry for pinging / mentioning the wrong user :)
Years ago, there was a lengthy discussion and tentative plan for an API like addMemoryForWrite(buffer, length) and addMemoryForRead(buffer, length). I believe @facchinm created a prototype for the HardwareSerial class. Then effort when into the RingBuffer class within the SAMD core library. But sadly, none of this ever became a public API to allow users to expand buffering from Arduino sketches & libraries.
Maybe a decision was made to abandon that API idea? Or maybe it was forgotten with all the Arduino dot org & Federico Musto conflict? Or perhaps runtime addition of memory buffers is still the general design goal, just a much lower priority than so many other developments?
Anyway, before this is merged, I hope the Arduino team will consider if compiler flag configuration really is the way Arduino's public API should develop. Compile time flags certainly are easy to add. They do result in the most efficient code at runtime, so there is a lot to like. If this is the way Arduino wants to develop, I am personally happy to see it. I only hope some real thought is invested in how this affects Arduino's long-term API design goals before this PR is merged.
I am confused how the ARDUINO_WIRE_RX_BUFFER_LENGTH
and ARDUINO_WIRE_TX_BUFFER_LENGTH
macros are intended to be used. There is no official mechanism to provide a global -D macro=value
flag to the C-preprocessor in the Arduino IDE. The CLI provides a hack with the compiler.cpp.extra_flags
property that barely works.
The user application cannot simply do:
#define ARDUINO_WIRE_RX_BUFFER_LENGTH 64
#define ARDUINO_WIRE_TX_BUFFER_LENGTH 64
#include <Wire.h>
because those new values will not be used to compile the Wire.cpp
file.
Any third party library that happens to #include <Wire.h>
will get the original values, not the new values. So we can get different translation units compiled with different macro values, which often leads to hard-to-diagnose bugs.
I'm using VisualMicro as IDE, and that allows global defines using the -D parameter.
Seems a bit strange to add a feature that is intended to work on one commercial plugin ($100), on one IDE environment (Visual Studio), on one operating system (Windows), on one microcontroller platform (AVR). But I think I can let that go, because it is actually a useful functionality... if the Arduino programming environment actually supported global macros.
My biggest concern is that this adds a footgun to an unfortunate segment of the Arduino population: the Intermediate level programmers.
<Wire.h>
doesn't work well for certain I2C devices that require large packet sizes.-D
override is required for this functionality.#define
just before the #include <Wire.h>
, because many 3rd party libraries actually recommend this unfortunate technique, and this can lead to obscure problems.So before this PR is merged, I would think it would be better if the Arduino IDE and CLI actually supported a proper -D
override functionality in some form, so that all users, on all operating systems, on the default Arduino programming environments, can take advantage of this flexibility.
Secondly, I wonder if the PR authors considered an alternative to using a global #define
. The alternative technique is to use dependency injection of the RX and TX buffer resources into the TwoWire
object, through the begin()
methods, using default arguments like this:
[Edit: Need to add rxLength
, and txLength
below.]
class TwoWire : public Stream {
public:
[...]
void begin(uint8_t* rxBuf = rxBuffer, uint8_t* txBuf = txBuffer);
void begin(uint8_t address, uint8_t* rxBuf = rxBuffer, uint8_t* txBuf = txBuffer);
void begin(int address, uint8_t* rxBuf = rxBuffer, uint8_t* txBuf = txBuffer);
private:
uint8_t* rxBuf_;
uint8_t* txBuf_;
};
This is not as efficient as referencing the rxBuffer
and txBuffer
symbols directly in the TwoWire
code, because of the extra level of pointer indirection through rxBuf_
and txBuf_
. But the class becomes more flexible, and I think this technique does not depend on having a -D
override functionality.
One further speculation, I wonder if using this dependency injection may solve an existing problem with the <Wire.h>
library that the compiler/linker is currently unable to optimize away the extern TwoWire Wire
instance even if it is never used in the application. In other words, simply adding a #include <Wire.h>
currently adds ~1300 bytes to the flash size, even if nothing uses Wire
.
Seems a bit strange to add a feature that is intended to work on one commercial plugin ($100), on one IDE environment (Visual Studio), on one operating system (Windows), on one microcontroller platform (AVR). But I think I can let that go, because it is actually a useful functionality... if the Arduino programming environment actually supported global macros.
As a counterpoint to this: We build products based on the Arduino AVR core (that ship with firmware source) and I would have been able to use Wire
unmodified if I'd been able to simply add a -D
to the boards.txt
build.extra_flags
for our product.
It's just one example that I picked, I know there's a project to use make files for Arduino-Projects and there it works aswell. So it's rather a missing feature of the Arduino IDE.
So I understand that your concern is that people will write a define in the first line of their .ino and break things that way if a library includes the files aswell. That can also happen if there's support for defines in the Arduino IDE if someone doesn't know the -D parameter is the way to go. So that issue would not go away then.
Arduino has been so successful because the software is easy to learn. Much of the design that makes it so easy to learn is intentionally leaving out many traditional features which add complexity to the learning process.
Just because an option is available (and this one would be really hidden) it doesn't mean it needs to be used. There is no reason why "under the hood" there shouldn't be any advanced features. There is other software which allows programming by connecting blocks together graphically, that's easier than Arduino but not as successful. So part of the reason is also, that it doesn't "block" semi-professional use cases.
I'm still very much in favour of this PR, although it hasn't been mentioned in two years, I think there's still point to either merge this or close it when the community disagrees on the added value.
All committers have signed the CLA.