Nano Every (ATMega4809) + Due (SAM3) support and some HAL restructuring

[OwlvilleWol]

Hi Nathan,

Here's my monstrosity. I'm not convinced that my architecture changes would make it better, so maybe it should be a separate thing, but I really don't want to deal with the package management, so here we are.

1. Nano Every was working before too,
2. I cleaned up the Due code. The basic idea is that it doesn't have to be low level anymore as the MCU has enough capacity to power through heavier stuff, thus the relevant functions are just wrappers around the factory Serial and SerialUSB library functions. The one magic trick used is that via a macro gcc demotes the ISR routines to regular functions which will be called whenever Camino wants to read or write, or if there's a buffer overflow, on the next main loop cycle.
3. Camino.cpp went back being a cpp (without any significant mods), and I added an arch.cpp too. There is still code in arch.h, and may not be a lot more readable, but at least not inside a preprocessor macro. The fundamental problem is that the Arduino environment doesn't allow the library and the user code to mingle at compile time. One solution is, as you did it, to put everything in the user code by including it in the header files. This here instead leaves as much as possible in the library part, just implements minimum necessary snippets in the headers imported from the user code and relies on extern declarations in the library to pick up user configured code during linking. On the AVR versions there should be one or two extra clock cycles tops overhead. E.g. the ISR routines take an extra instruction due to the aliasing method, or instead of #defining the buffer array size, the whole buffer is defined in the user header, and linked together as an extern variable reference. The hearder files also check if they were included from library or user code. It could be split, but this is more compact.
4. I renamed the PORT macro as it had a conflict with a factory library used for the SAM3.

I didn't touch the original AVR arch code as I can't test it. I ordered an Arduino UNO and a Mega 2560. Considering how deep I am in the rabbit hole with this I might as well complete it.

[OwlvilleWol]

Now I feel a bit better about this. Using the UNO R3 and the Mega2560 I reworked the sections for the older devices too. I also separated the arch.h into arch.h + arch_user.h. Latter contains definitions influenced by user code macros. acrh.cpp, arch.h, camino.cpp, camino.h, options.h are pure library code.

Test.ino + tesy.py passes on the: Nano Every - USB Port DUE - Native USB Port DUE - Programming USB Port UNO R3 - USB Port Mega 2560 R3 - USB Port (I didn't try any of the TTL serial pins, but theoretically they should work.)

For the older AVR devices I changed the deprecated macros to the new style. E.g. sbi, cli This way "wiring_private.h" doesn't have to be included.

[n-wach]

Thanks, lots of improvements here. I'll have time to review it in detail this weekend.

At first glance, it's a little hard for me to follow all the externs/files.

I'm wondering---what's the justification to bring back .cpp files? I know that header-only libraries are bad practice, but I thought the downsides only surface in larger projects. It seems like a header-only library would make this a lot cleaner

[OwlvilleWol]

Let me start with that I'm not an expert by any means. I'm an engineer not a CS person. The header only way isn't necessarily bad, I guess what I disliked the most is having to put significant chunks of code into macros. It would be entirely possible to declare functions for every variable piece and just call the right one picked by an #ifdef tree. And at that point if you have small sections as functions, you might as well declare them extern and implement them in a different file. The value in the library is obviously the comms part, with packet handling, callable management, timeouts etc. I wanted to keep that as "normal" as possible. In a sense of "hey here's this awesome library: Camino.cpp + Camino.h" this is what 99% of people care about and it looks feels smells quacks like some normal vanilla c++ library. "and then there those other files included, it's because the Arduino environment is ... special, but this way the library still provides customization options." This gets philosophical at this point, and it's your code. Maybe in order to drop one file, arch_user.h could be merged into options.h, that way one can say that everything that is related to user parameters is in one header file and the naming is less confusing also. IDK.

[OwlvilleWol]

I did those changes. Maybe a bit better. Moved arch_user into options.h Also got rid of some of the cross includes and associated checks between camino.h and arch.h One thing I don't know what to do with is the macro IN_CAMINO_LIBRARY that's used to tell whether camino.h was included from camino.cpp or from the sketch. If latter, camino.h will will include options.h I wish there was a preprocessor directive to easily tell apart the two cases without a prior #define

[n-wach]

I am also no C expert, thanks for bearing with me (:

I think you're right to split into the user-included Camino.h header, and actual cpp library. My main gripe is also with the IN_CAMINO_LIBRARY macro, which stems from reusing the same Camino.h in the cpp implementation. Here's my idea:

The camino library Camino_lib.cpp includes its own header Camino_lib.h. This header file declares all options-driven variables/functions extern.

A user #defines any options, and then #includes "Camino.h". This Camino.h contains the implementations for all the extern variables/functions in Camino_lib.h. That's mapping the ARDUINO_xxx/PORT to the correct InitPort implementation; setting the byte packetDataArray to the right size; etc., as you've already done.

So we'd have three files total: external Camino.h; internal Camino_lib.h/Camino_lib.cpp

Do you think this would work? I'm unfamiliar with ISR_ALIAS and if it works with this scheme

[OwlvilleWol]

Yep, that sounds great. Merging the library cpps and hs will make some of the back and forth includes less confusing. I've been thinking more about this and I think if there was a proper way somebody would've done it by now. I played around with the thought of defining function templates in the library and letting the compiler create every possible version (e.g. for all serial ports) then via only picking one during user code compilation, the linker would throw out the rest. Then I realized this wouldn't work with buffer sizes and the other options.

I also think I made a mistake in my last commit. I hastily merged stuff into options.h and didn't test with everything after. (classic) This might've made the ISR_ALIAS more confusing, because I somehow left out an explicit declaration of the vector functions being extern to the user code.

To address your concern ISR_ALIAS should work just fine, but now thinking about it probably it can be done simpler. In camino.cpp I replaced ISR(USART_UDRE_vect) with ISR(Camino_ByteSent_vect). On platforms where the ISR macro is not defined (ARM) it will simply turn into a regular function: void Camino_ByteSent_vect() On AVR it will still create an ISR routine, but it will put it at the end of the vector table with no actual interrupts belonging to it. ISR_ALIAS is normally used to allow multiple interrupts being handled by the same routine. Let's say you do ISR(USART0_UDRE_vect) then use ISR_ALIAS(USART0_UDRE_vect, USART0_RX_vect) to tell the build chain, oh btw call the same function on this other interrupt too, instead of putting a new entry into the vector table. In my case the difference is that the first interrupt is not real thus could never happen, only the one I aliased to it. Maybe a less confusing possibility would be to just make a simple function in camino_lib.cpp: Camino_ByteSent then entirely define the ISR in camino.h as extern void Camino_ByteSent(); ISR(USARTx_UDRE_vect){Camino_ByteSent_vect();} Doing it with ISR_ALIAS made sense considering I didn't want to mess up the older AVR specific code. The Microchip website says that since gcc 4.2 there's an ISR_ALIASOF macro, which if I understand correctly is able to do the aliasing without any overhead as opposed to the extra call with ISR_ALIAS, but I couldn't make ALIASOF work, nor that single instruction really matters, so just defining the ISR in the user header and calling the handler function in the library would be probably less confusing.