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

Event system and Arduino Pin numbers #126

Closed
aikopras opened this issue Aug 17, 2021 · 51 comments
Closed

Event system and Arduino Pin numbers #126

aikopras opened this issue Aug 17, 2021 · 51 comments

Comments

@aikopras
Copy link

I started playing with the Event System to decode manchester coded input. I configured the Manchester coded input pin as Event generator, and TCB0 as Event user. The timer is configured to raise an interrupt, after which the ISR reads the input signal.

Things work very well, and using the Event System is (thanks to the provided library) quite simple.

There is one thing that (for me) is a bit annoying, and that is that the Event System / Library expects as input Port and Pin numbers, such as gen0::pin_pa0. For me it would be easier if instead of pin_pa0 the "standard" Arduino Pin number (2) could be used.

The question is thus if/how this is possible. I know that I could write some switch or if statements to create such mappings, but that solution would be variant specific.

@MCUdude
Copy link
Owner

MCUdude commented Aug 20, 2021

Great to hear you find the Event system useful and the library easy to use!

I'm planning to rewrite part of the Event library in cooperation with @SpenceKonde, and Arduino pin numbers are one thing that will be implemented. The other things that need to be done are mostly about easier integration, so other libraries easier can build upon the Event library.

@SpenceKonde has used the library more than I have (even though I'm the author), so he will be providing a list of necessary changes.

More info here:
SpenceKonde/megaTinyCore#480

@MCUdude
Copy link
Owner

MCUdude commented Aug 20, 2021

After looking a bit deeper into to the "Arduino pin as generator" issue, we can't just pass an Arduino pin number to set_generator(), because the object(Event0...Event7) has to support that pin as a generator. A solution to this is to let the library select the object for you, like so.

// Use Arduino pin PIN_PA0 as event generator, and let the library find
// the lowest channel number that supports PA0 as event generator
Event& myEvent = Event::set_generator_pin(PIN_PA0);

// Set PF2 as event user (yet to implement Arduino pin
// numbers for users
myEvent.set_user(user::evoutf_pin_pf2);

// Start Event generator
myEvent.start();

// Check channel number in use, just for fun
Serial.printf("PIN_PA0 uses Event channel %d\n", myEvent.get_channel_number());

// Check object in use
if(&myEvent == &Event0)
  Serial.printf("PA0 uses Event channel 0");

@aikopras
Copy link
Author

First I would like to thank you (as well as SpenceKonde) for the fantastic things that you do. I can give many examples where your work helped me a lot, both with older processors, like the Mega16 (MightyCore) as well as the newer processors (MegaCoreX, DxCore).

I'm also happy to see that I'm not the only one with this request, and that SpenceKonde and you are considering rewriting the Event library.

Yes, I do realise that channels and ports are tight together, which indeed requires the library to decide which event channel will be used for a given pin. To a certain extend the library user needs to be aware of that, and understand that (like for example USARTS) there is not complete freedom in choosing any pin you like.

To be honest, I find the pin numbering for Arduino boards (like the Nano Every) quite confusing, but the numbering you propose for the (48, 40, 32 and 28) pin variants much more logical.

PS: Are SpenceKonde and you also considering to change the Logic library such that Arduino pin numbers can be used?

@MCUdude
Copy link
Owner

MCUdude commented Aug 21, 2021

Thanks for the kind words! I've been doing this for a while now, and I've definitely learned a lot from it! In my opinion, it's very liberating to know that pretty much any hardware that has an AVR can be programmed with Arduino using either mine or SpenceKonde's cores.

To be honest, I find the pin numbering for Arduino boards (like the Nano Every) quite confusing, but the numbering you propose for the (48, 40, 32 and 28) pin variants much more logical.

Yes, the Uno Wifi Rev2 and especially the Nano Every has a pretty wild pinout because they try to make them "as ATmega328P compatible" as possible. What still surprise me is that they have not at all provided all the features the Nano Every really has to offer. The official core only implements two serial ports, but the Every has four. The Nano Every also has way more analog inputs than the official Arduino core provide. Since I personally never really use Arduino shields, I don't rely on pinouts that mimics the functionality of ATmega328P based Arduinos. That's why I've made all the pinouts as straight forward as possible, and why Spekce Konde followed the same exact pattern as me with DxCore.

Yes, I do realise that channels and ports are tight together, which indeed requires the library to decide which event channel will be used for a given pin. To a certain extend the library user needs to be aware of that, and understand that (like for example USARTS) there is not complete freedom in choosing any pin you like.

If you look at the Event system in the datasheet, you'll pretty quickly realize that hretty much every application that utilizes the Event system has to be talor made to even get the Event system to work. Writing a high level library that would magically just take care of everything is pretty much impossible, especially since the Event system is so quirky. Arduino as event generators and event users makes total sense, and is absolutely possible to implement in a straight-forward and logical way. However, I'm not so sure it's that easy when it comes to all the other peripherals like timer, logic etc. But I'm open to suggestions on how it could be implemented!

PS: Are SpenceKonde and you also considering to change the Logic library such that Arduino pin numbers can be used?

I'm not sure I'll add Arduino pins to the Logic library, and it would be way more work that with the Event library, since none of the input pins can be moved around. My experience is that if you want to use the Logic hardware you'll first have to figure out what you want to accomplish, then pick one or more logic block to use, and you're pretty much stuck with the pinout and output pins the block(s) has.

@aikopras
Copy link
Author

I'm not sure if I should better react here or on SpenceKonde's issues page.
Anyway, at that page the question was raised about what requirements a modified Event library should satisfy. I can't say what others want to do, but maybe my ideas help. If not, please ignore.

Context: I'm building (as hobbyist and open source) decoders for modeltrains. Such decoders often rely on the DCC standard for receiving messages. The DCC message is manchester biphase encoded. Signal transitions (a pin interrupt) take place roughly every 100 microseconds, after which the pin ISR starts a timer which fires 75 microseconds later (the second Interrupt) to read the signal's value. Of course other decoding approaches are possible.

The event system can be quite useful to connect the pin Interrupt to the timer.
The advantage is not only better / more predictable performance, but it saves the first ISR that gets triggered after the signal transition on the input pin. The saving of an ISR is particularly important with the new megaX and DX core processors, since their interrupt system was changed (from one interrupt per INTx pin at the 328 and others to one interrupt per port). SpenceKonde has a nice description of this https://github.com/SpenceKonde/DxCore/blob/master/megaavr/extras/PinInterrupts.md,. If I measured well this new interrupt system takes (using attachInterrupt()) roughly 6 microseconds to get started, compared to roughly 1 microsecond with the 328/2560). For my application the overhead that these two ISRs generate every 100 microseconds is substantial.

However, things get worse since I also have a "feedback bus", where a master polls all (128) slaves. One polling puls takes roughly 100us, and after each puls a slave has to update its pulse counter.

So this brings me to a third interrupt every 100 microseconds. Evey 100 microseconds I seem to loose roughly 15 to 20 microseconds to the overhead of three standard attachInterrupt() routines. Thus for me the real motivation to use the event system is to get at least similar interrupt efficiency with the new cores as with the old cores.

Of course I could replace attachInterrupt() and write my own pin interrupt routines. The danger of doing that, however, is that if the user of my library does use attachInterrupt(), a conflict occurs. For me a much cleaner approach seems to let the Event system generate an interrupt, and write myself an Event interrupt routine instead. For me this sounds much safer then replacing attachInterrupt().
Instead of the Event system also the Logic library seems interesting, since it seems that also the CCL can be used to generate interrupts for which I can write the ISR handling routine myself. So for me the CCL seems interesting to avoid attachInterupt().

I hope this helps.

@aikopras
Copy link
Author

Thanks for the kind words!

For me it is fantastic what people like you do for the community!!!

it's very liberating to know that pretty much any hardware that has an AVR can be programmed with Arduino using either mine or SpenceKonde's cores.

Yes, and the cores provided by you and SpenceKonde are better than the standard cores.
Btw, one wish that I still have for some older cores of you: it would be nice to have Fast IO also there.

Arduino as event generators and event users makes total sense, and is absolutely possible to implement in a straight-forward and logical way. However, I'm not so sure it's that easy when it comes to all the other peripherals like timer, logic etc. But I'm open to suggestions on how it could be implemented!

Is your question how to connect timers etc. to the Event Library? But that seems trivial: Event2.set_user(user::tcb0). What more is needed? For many people the challenge will be the writing of the timer initialisation code. There are some libraries out there, but my feeling is that many of them focus on PWM generation. If you want something simple, like the case I described in my previous post, I just needed single shot mode.

@SpenceKonde
Copy link
Contributor

SpenceKonde commented Aug 21, 2021

Hans - YES EXACTLY!

This is what I have been saying since I first commented on thls library around when it got merged into DxCore.... and which I started shouting much more loudly about since I made that issue, because that's the first time I tried to actually use it to write some INCREDIBLY BASIC event system code in a non-part-specific way to make one of the simplest use cases happen) and realized that it simply wasn't possible without ugly brute force solutions if I wanted the code to run everywhere.

In order to be able to do anything in the general case, you cannot use the Event# objects, because you don't know which one to use! (even if you did, there is no way short of a switch case statement or lookup table to get from the integer 1 to the Event object named "Event1"), the only way this shit is gonna fly is if the library is capable of handing us an event channel with the specified properties.

I strongly disagrtee with the claim that this is a fools errand because the EVSYS peripherals are different from eachother or something. With the exception of the tinyAVR 0/1-series (aka "public beta"), and the way that 2-series pins wrap around while the 48/64 pin parts just have some second class channels that can't do any pin everts, they're all same exact thing, just different lists of users and generators.
Getting rid of the "how do I go from a pin to an EventN object?" barrier would get rid of one of the big things that is keeping people from using this new functionality. Anyone wanting to do anything that requires an event channel needs to solve this problem first or make a single-part-only library, and this problem is a much bigger deal to solve than the rest of what they'd probably want to do with the event channel. And so unsurprisingly nobody has written libraries that use event channels to do awesome things, or even to do lame things.

There are a lot of cases where the only way you can do something is to "use an event channel to connect pin X to event input Y"
But how to make the whole thing coherent, there are a lot of design questions, a lot of decisions to make, and I have been flat out and have not had time to give this the thought it deserves and requires.

The reason Arduino isn't making their hardware work to it's full potential is nothing new. The mega2560 can do differential ADC.... with 200x gain on some pin combos! But Arduino doesn't give you any facilities for that (My cores do, as it happens, because I knew if I didn't do something about it, it would never happen for 2-series, and 2-series is all about the ADC). The Arduino people have never have done that kind of thing - that's what the community contributes :-P . They are not gifted electrical engineers nor competent embedded developers. They made an "good enough" IDE with an incredibly easy learning curve. which is a really hard UX nut to crack, and they have that board manager system to install toolchains. I know of plenty of people installing the IDE to get board manager just to install toolchains that they call using other methods - that's another hard thing they've dcone. And. and they do an incredibly good job of outreach and evangelism. But their hardware has always been of mediocre design (and it hasn't gotten better), and their cores, well.... there's a reason we have our own copy of the core ;-)

@aikopras
Copy link
Author

The reason Arduino isn't making their hardware work to it's full potential is nothing new

Yes, I completely agree. Seems that Arduino, and many of its developers, go for "generic code", which runs more or less on each and every platform, ranging from Arduino Uno to 32 bit RISC processors and ESP. The performance cost of making everything "generic" may be acceptable on high end processors, but killing for slower processors, such as the ATMega's. By not taking advantage of the novel functions in these newer ATMega processors, we get none of their advantages (like the Event system) but all of the disadvantages (like the performance penalty of attachInterrupt()). Why would someone design new hardware for a 4809 or 128DA, if the performance at the end will be lower.

It would be great if some "standard" libraries would be there to use novel functions, such as the Event library.

@MCUdude
Copy link
Owner

MCUdude commented Aug 21, 2021

I'm about to push my current work to a new branch so you guys may have a chance to look at it. The code compiles and runs fine, and the README is also updated with the new functions. I'll have to at least create one new example to illustrate some of the new functionality, but it's already getting pretty late here.

We can now let the library choose a suitable Event channel for you, and Arduino pins can now be used as event generators and users. The following code compiles and works fine:

  Event& myEvent = Event::set_generator_pin(PIN_PA0);
  myEvent.set_user_pin(PIN_PC2);
  myEvent.set_user_pin(PIN_PD2);
  myEvent.start();

Btw, one wish that I still have for some older cores of you: it would be nice to have Fast IO also there.

I've looked into adding digitalWriteFast() and digitalReadFast() to my classic AVR cores, but I haven't been able to make it compile down to a single instruction without basing the whole thing on a ton of macros. I'd much rather prefer to use the pins_arduino.h pin mapping if possible.

Is your question how to connect timers etc. to the Event Library? But that seems trivial: Event2.set_user(user::tcb0). What more is needed? For many people the challenge will be the writing of the timer initialisation code. There are some libraries out there, but my feeling is that many of them focus on PWM generation. If you want something simple, like the case I described in my previous post, I just needed single shot mode.

It is trivial to pass user::tcb0, but Spence mentioned something about passing the TCB0 struct itself, not just a constant like user::tcb0 is. I'm not sure how that could be implemented in a straight-forward way.

@SpenceKonde; functionality vise, is there anything obvious you think is missing now?

The updated library can be found here:
https://github.com/MCUdude/MegaCoreX/tree/New-Event-library/megaavr/libraries/Event

@aikopras
Copy link
Author

Hi Hans

I've just been playing with it a while, and it works very nice. For my application the code is now just a few lines:

  Event& myEvent = Event::set_generator_pin(15);
  myEvent.set_user_pin(5);
  myEvent.set_user(user::tcb0);
  myEvent.start();

I only need to know the Arduino pin numbers, which nicely integrates with the remainder of my library code.
What I also enjoy that the Event system reacts on inverting the pin input (using PINnCTRL).

Thanks a lot, this really helps to make my code nicer and smoother. I hope others will enjoy this as well; attached my sketch as example.

If I have additional questions or feedback I'll let you know.
Thanks, Aiko

/***********************************************************************|
| megaAVR event system library                                          |
|                                                                       |
| Simple_event_with_timer.ino                                                      |
|                                                                       |
| A library for interfacing with the megaAVR event system.              |
| Developed in 2021 by MCUdude                                          |
| https://github.com/MCUdude/                                           |
|                                                                       |
| In this example we use the following pins and timers:                 |
| - PD2 (Arduino pin 15): event generator                               |
| - PB2 (Arduino pin 5): event user (follows PD2's state)               |
| - TCB0: event user                                                    |  
| - PIN_PE0: In main loop to indicate if the timer is running           |
| - Pin PE1: Set and cleared by the timer                               |                  |
|                                                                       |
| See Microchip's application note AN2451 for more information.         |
|***********************************************************************/
#include <Event.h>

#define TIME_US 50    // Time (in microseconds) before the timer triggers


void init_timer(void) {
  noInterrupts();
  // Step 1: Calculate from TIME_US the right value for TOP
  #define TOP F_CPU / 1000000  * TIME_US
  // Step 2: fill the registers.
  // We use single shot mode
  // Reset the main timer control registers
  // This may be needed since the Arduino core creates some presets
  TCB0.CTRLA = 0;
  TCB0.CTRLB = 0;
  TCB0.EVCTRL = 0;
  TCB0.INTCTRL = 0;
  // Initialise the control registers 
  TCB0.CTRLA = TCB_ENABLE_bm;                    // Enable the Timer/Counter type B peripheral
  TCB0.CTRLB = TCB_CNTMODE_SINGLE_gc;            // Single-Shot mode
  // Event system: enable input capture events
  // Start counter only on positive edge
  // Enable noise cancelation
  TCB0.EVCTRL = TCB_CAPTEI_bm | TCB_FILTER_bm;
  // TCB0.EVCTRL = TCB_CAPTEI_bm | TCB_EDGE_bm;
  // Enable CAPT interrupts by writing a ‘1’ to the ENABLE bit in the Interrupt Control (TCBn.INTCTRL) register.
  TCB0.INTCTRL |= TCB_CAPT_bm;
  // Write a TOP value to the Compare/Capture (TCBn.CCMP) register.
  TCB0.CCMP = TOP;
  interrupts();
}


void setup() {
  // Pins for debugging
  pinMode(PIN_PE0, OUTPUT);          // Timer is running
  pinMode(PIN_PE1, OUTPUT);          // Is set and cleared within timer ISR
  digitalWriteFast(PIN_PE0, LOW);    // Initialise
  digitalWriteFast(PIN_PE1, LOW);
  init_timer();
  //
  // Event system, with Arduino pin numbers and an unknown event number
  // PD2 = Arduino pin 15
  // Uncomment to invert the input pin (TCB starts on falling edge)
  // PORTD.PIN2CTRL |= (1 << PORT_INVEN_bp );     // Invert the Interrupt
  Event& myEvent = Event::set_generator_pin(15);
  myEvent.set_user_pin(5);
  myEvent.set_user(user::tcb0);
  myEvent.start();
}

// The ISR should be called explicitly
ISR(TCB0_INT_vect) {
  TCB0.INTFLAGS |= TCB_CAPT_bm;      // We had an interrupt. Clear!
  digitalWriteFast(PIN_PE1, HIGH);
  digitalWriteFast(PIN_PE1, LOW);
}
    
void loop() {
  // PE0 indicaqtes if TCB0 is running
  digitalWriteFast(PIN_PE0, TCB0.STATUS);
}

@MCUdude
Copy link
Owner

MCUdude commented Aug 24, 2021

@aikopras Thanks for taking the time to test out the revised Event library.
Great to hear the latest revision makes it easier to integrate with Arduino projects.

But by looking at your code, it's pretty clear to me that a TCB library also would have been nice to have; written "in the same flavor" as the Event library.

I'll wait for @SpenceKonde's feedback before merging the updated library into master though. He promised he would get back to me about this.

@aikopras
Copy link
Author

But by looking at your code, it's pretty clear to me that a TCB library also would have been nice to have; written "in the same flavor" as the Event library.

Sounds quite interesting. Despite the complaints that others sometimes have, in my opinion the new TCB timers have some interesting features that the old timers didn't have. This is even more true for the DxCore.
Can you share any ideas how this may look like?

@MCUdude
Copy link
Owner

MCUdude commented Aug 25, 2021

Can you share any ideas how this may look like?

I haven't really looked into it at all, it's just a thought that a library might be nice to have and doable to create.
The idea was to simply create a higher-level wrapper (like Event is for the event system) that makes it easier to interface with the TCB timers. There is a little more info here: SpenceKonde/megaTinyCore#481

@aikopras
Copy link
Author

I've been working now for a while with the new Event Library on 4808 and 4809 chips, and I must say that I really like it. It is extremely simple, but therefore very powerful. Since I'm including it in one of my libraries, I would enjoy if this new event library will be included in the next release of MegaCoreX.

I've also tried to "blindly copy" this library to the DxCore, but that didn't work. I guess it is something simple, but I didn't dive deeper. What I noticed, however, is that the Timer Event users on the AVR DA and DB series have a slightly different name: instead of tcb* it is now tcb*_capt (there is also a new and interesting tcb*_cnt). To enable portability between both cores, wouldn't it be an idea to have a #define that works on both cores?

Thanks a lot for developing this great library!!

@MCUdude
Copy link
Owner

MCUdude commented Sep 14, 2021

@aikopras great to hear you like the library! The plan is to merge it into master, but I was really hoping that @SpenceKonde could provide some feedback and/or suggestions on how to further improve the library. Ideally, the new library will be ported to megaTinyCore and DxCore, so a user could expect the same features regardless of which core is being used.

@SpenceKonde have you lost interest? You're not responding to anything I write these days...

@SpenceKonde
Copy link
Contributor

SpenceKonde commented Sep 14, 2021

No I have not lost interest, Indeed as I've said, it is one of the most important libraries to get right within the entire modern AVR architecture, and if it catches on at all will impact Ardiino fpr many years. But my users disagtee, and say that Wire is a much higher priority, what's the event system,, ,my projerct doesn't work because ofall these I2C problems..... I had made a rule to not work on anything else of consequence until I had a usable version of megaTinyCore in the 2.4;x versions out,and which I expected would be my main project after ordering my june order of PCBs (placed in august), Both of those got shoved out of the way when a freelance client needed some super urgent work done for his next version (they have a pretty slick product, though it isn't quite as slick with all the DNP pads on the PCB for parts that they wanted to include, we designed for, but he's been utterl;y unable to source with expected ship dates before the newyear. ..

Anyway. I figured that it would take at most a few weeks to release 2.4..since we had just gotten SerialUPDI working with lhe latest optimizations. but here we are in september, with that goal having just finally been achieved. I want to get the new modernAVR Wire library squared away in the github version of megaTinyCore (two issue to clarify with author, and minor change that makes no difference on tinyAVR. but I want (really, I need) to be able to directly copy files between the cores. rather than going through some porting process. That update adds both Wire1 support and support for using both master and slave simultaneously (either in dual mode if supported or on the same pair of pins), and does everything in less flash (but more ram if you want master+slave - since the master transaction methods just queue up stuff, rather than actually changing the bus state,so , nothing precludes, in such a multimaster system,there being a slave TWI event between beginTransaction and endTransaction and hence you can't share the buffer). This is a change that people have been asking for in both cores both via email and github, I don't really understand WHY people want it so badly, but it is a very common request which I was totally out of my league trying to implement). Considering the history of Wire.h library implementations in my cores thee is need for a ton of real world testing, so I need to get the new wire library in the github version of both cores (megaTinyCore's will be in now, but DxCore gets one "safe" release with a few bugfixes, before the release that will actually include the new Wire library just like 2.4.2 mTC. So I'll deal with those bugs, then I release and put a version that I am confident in out for users, But that is a change that I want in the hands of people as long as possible before it is in a release both on megaTinyCore and DxCore) because it is intended to be a drop-in, smaller replacement., yet CI tells us nothing about its correctness, and there is so much hardware and so many interactions that could break in so many subtle ways....So I want to get the completed version for megaTinyCore (which will naturally be ready to drop into next DxCore release at that point, assuming (it's author has done some testing at leas.)

After that I get to put a little time into modern AVRs which is where I hope to get Event library examination in at long last, and fix a couple of other hot issues like the analogWrite problem and squeeze out a DxCore patch, that fixes a few important bugs and adds the expanded device support top Serial UPDI, potentially updated event depending on how it's looking, release, check in new Wire, and run to ATTinyCore and get 2.0.0 (80% done I think) fucking released - it's a huge restructuring of the core on every level., that core existed under my auspices before I had clue what hell I was doing, and it's bad in so many stupid ways. I have owed them a release for 9 months now , and for most of that time all the pins_arduino files have been redone, there's sa new implementation of and people who only use ATTinyCore have been starting to wonder what happened to me. And it's most recent release (as well as every one prior to that) not something I'm particularly proud of. (I was considerably worse than I am now then)..

Have you been watching the recent releases of DxCore and megaTinyCore? There is a bunch of stuff that I imagine you would want to steal for mega 0-series parts.... tuned oscillators not just for accuracy but to over/underclock when precision of ext clock not required. , Runtime changeable PWM pins for TCA (I didn't bother to figure out some way to implement it for the apparently randomly scattered TCB's if it's even possible with acceptable overhead. Considering what crapola timers they are it probably isn't)., then analogReadEnh() (enhancedAnalogRead() and the functions associated with that), the for for the bug with long tones at high frequency,. and the bug with delayMicros() screwing up when the function gets inlined (unless you;ve modified it, it relies on the call and return overhead to get the time right (also, I don't think it was updated on official cores to account for the slightly altered timing of call, nor does it cover a bunch of relevant frequencies that are suddenly within reach through tuning. And there's a pile of #defines I use so that third party code can detect if it's on a core that supports a given feature. Which, ah., probably matters the most for you, since someone writing a library for a 4809 could be dealing with official core or your core, and they would want to know, for example if they could use fastDigitalIO (CORE_HAS_FASTIO, or knowing nothing about what part they're running on, check ADC_MAX_OVERSAMPLED_RESOLUTION to get the highest resolution possible on that part with a single analog read operation followed by decimation of the result (analogReadEnh() does that automatically) or if it's not defined, then they'd have to "manually" oversample (which would be a shame if the part actually supported it, but the organization of the registers is inconsistent) - you can ask enhanced analogRead for up to the maximum number of bits math says you can extract that might be meaningful from the accumulated result - it the case of mega0, that's 13 (native resolution, plus half the extra bits added by the accumulation (which have increasing cost, of courser 1 bit costs 4x time taking the reading 2 bits cost 16x the time, and the limit on these of 3 cost 64x the time (the maximum oversampling option). On the new parts with the fancy adc which is native 12b and proper differential, programmable gain and all that shit, on the other hand, you can take 1024 samples (which can be a substantial amount of time It strikes me that a way to call either of those functions and then return as soon as it's started taking samples, so it can terd to other tasks while the ) to add 5, giving a 17-bit measurement. Note that against anything other than noiser, it just gives false confidence that the fixed offset error which is the same every time you measure is actually real part of the data. I thought I was going nuts at first but then reazed that I was measuring a differential voltage so small with the minimum size reference that the results were dominated by offset error (I also had a bug at that time that incorrectly set tht ADC clock to the maximum, so the ADC was waaaay overclocked , fixing that helped.....It seens likeit's been a long time since I've heard interest in taking a feature or rven a fix back to your core. Are you still interested in that? Surely you're going to want the new Wire when it's ready!

@MCUdude
Copy link
Owner

MCUdude commented Sep 19, 2021

In short terms, The Event library is very similar to the previous version, but it now supports Arduino pins in as generators and users, and you can get hold of the right object by either asking for a channel number, event user or event generator. This is what makes the library flexible.

@SpenceKonde Wire/i2c issues? I haven't got any Github issues regarding this. Am I just lucky, or is this something that only affects the tiny0/1/2's? I'm usually very hesitant to replace "backbone libraries" like the Wire library is. If it breaks, a load of Github issues will pile up! But it it's worth it, of course!

Have you been watching the recent releases of DxCore and megaTinyCore?

Yes, I've regularly had a look at them from time to time. There are absolutely nice features I should get around to implement, but I do disagree on the implementation part of some of them. The ADC is a powerful and "important" peripheral, and all the "advanced" features could IMO be split out in a library instead, perhaps a static one for improved efficiency and code size.

Have you experienced any issues regarding all the pin related compile time checks? I implemented this in MicroCore a while ago, but I had to remove it due to the compiler being difficult.

@SpenceKonde
Copy link
Contributor

Finally having a chance to look at this this week for 1.3.7 DxCore, now that I've finished with a huge blob of fixes and improvements over there (take a look at what I did to the boards.txt - the file is B E A U T I F U L now (the initial impetus was to make editing easier with text editors that have the "minimap" scrollbar functionality.

You haven't been getting complaints about I2C?!! The baud rates were totally fucked, still. We went through two complete rewrites of the master baud rate calculation since the last time that came up and we were in communication about it, before arriving at one that works over on my cores. I think that brings the total rewrite count for baud rate calculation up to at least 5.

I think at least two of the 5 open issues about I2C on my cores are user error, as the totally new implementation and the old one had the same behavior. One thing that is very clear is that people are not very good at debugging I2C or knowing how to approach I2C issues. The maximally ungraceful response to the bus being hung up by something else always directs attention to the core (nobody ever thinks to measure the voltage on the SCL and SDA lines - Isn't that the first thing you do? Then you unplug the one that's being held in the wrong state from one side orr the other, and observe that it jumps back up to normal on the Arduino side while the other device continues holding it low, and conclude "Crap, I must be doing something that's making the device unhappy").

I also was constantly getting requests for dual mode and master+slave functionality. Do you not get those? @MX682X has blessed the Arduino community with what is essentially a complete rewrite of Wire with drastically reduces compile size, AND does dual mode and master+slave which is gonna be killer.

I will give a big long general update via email tonight, and will also be looking at this

As for the compiletime pin checks - for functions that do NOT require it (like the fast digital I/O) I only do the check if the pin is compiletime known (do you have LTO enabled? You must have LTO enabled for these checksto work reliably, apparently; on my modern cores I only support LTO. ATTinyCore 2.0.0 will also force it on)). Except for really clumsy and stupid bugs, nope, no problems at all with them. They have worked beautifully. Every time I have a chance to recognize input is constant and that it's bad, I will badCall() it and turn it into a compile error.

My philosophy is always "anything that can be recognized as guaranteed not to work at compile time, I want it to be a compile error, because we have no exception handling here, and debugging takes 10-20x longer than fixing a compile error.

Also, we've gotta figure out a consistent way to format the files - of course, that's "holy war" territory, so that we can easily merge changes between them and see what was changed. It would be awesome to end up with a single version of Event for all the parts that support it instead of one version for Dx, one version for tiny, and one for mega zero series. I wound up reformatting the copy I downloaded so I could diff it.

Can I ask what the comments before every function are, or rather what the spec is for it and how yhey're used? It looks likethere's some sort of auto-documentation generated fromthem, but I don't know what it is.

@MCUdude
Copy link
Owner

MCUdude commented Oct 19, 2021

You haven't been getting complaints about I2C?!!

Nope. But I'll look into the new library you guys have been working on when you're finished. I have been busy with other things lately.

I also was constantly getting requests for dual mode and master+slave functionality. Do you not get those?

Nope, not really. And I really don't see why one would need it really. I'm sure there are applications where this might be useful, but these have to be niche.

Also, we've gotta figure out a consistent way to format the files - of course, that's "holy war" territory, so that we can easily merge changes between them and see what was changed. It would be awesome to end up with a single version of Event for all the parts that support it instead of one version for Dx, one version for tiny, and one for mega zero series. I wound up reformatting the copy I downloaded so I could diff it.

I don't use a code formatter, I just format the code the way I prefer to structure and read it. If you're using an automatic tool for this, why don't just use my files and put them through the formatter?

Can I ask what the comments before every function are, or rather what the spec is for it and how they're used? It looks like there's some sort of auto-documentation generated from them, but I don't know what it is.

It is for editors that support Doxygen-styled comments, for instance, VSCode.

Here's a screenshot where the comment is nicely formatted when I hover the mouse over the function name:

image

But do you have any thoughts or comments on the Event library itself?

@SpenceKonde
Copy link
Contributor

I've been looking over it, I see three issues (beyond the fact that we need to come up with formatting conventions that leave the library readable and which we can both stand so that we can have one file for both both of our cores)

  1. I think set_generator_pin() is not a good name - the static set_generator_pin does something fundamentally different from the non-static set_something() member functions. I propose "assign_generator_pin" to capture that it does more than just configuring that pin, it also chooses a usable channel.
  2. assign_generator_pin should be able to return a currently configured channel if it's set to the requested pin.
  3. The last part is to take a generator instead of a pin number and do the same thing: assign_generator()

Here's my proposed fix to these three issues

Event.h:

    static Event& assign_generator_pin(uint8_t pin_number);
    static Event& assign_generator_pin(uint8_t port,uint8_t port_pin);
    static Event& assign_generator(gen::generator_t event_generator, uint8_t ch = 255);
    /* will this do what I think it will?? */
    #if defined(EVSYS_CHANNEL0)
      static Event& assign_generator(gen0::generator_t generator) { set_generator((gen::generator_t)generator, 0); }
    #endif
    #if defined(EVSYS_CHANNEL1)
      static Event& assign_generator(gen1::generator_t generator) { set_generator((gen::generator_t)generator, 1); }
    #endif
    #if defined(EVSYS_CHANNEL2)
      static Event& assign_generator(gen2::generator_t generator) { set_generator((gen::generator_t)generator, 2); }
    #endif
    #if defined(EVSYS_CHANNEL3)
      static Event& assign_generator(gen3::generator_t generator) { set_generator((gen::generator_t)generator, 3); }
    #endif
    #if defined(EVSYS_CHANNEL4)
      static Event& assign_generator(gen4::generator_t generator) { set_generator((gen::generator_t)generator, 4); }
    #endif
    #if defined(EVSYS_CHANNEL5)
      static Event& assign_generator(gen5::generator_t generator) { set_generator((gen::generator_t)generator, 5); }
    #endif
    #if defined(EVSYS_CHANNEL6)
      static Event& assign_generator(gen6::generator_t generator) { set_generator((gen::generator_t)generator, 6); }
    #endif
    #if defined(EVSYS_CHANNEL7)
      static Event& assign_generator(gen7::generator_t generator) { set_generator((gen::generator_t)generator, 7); }
    #endif
    #if defined(EVSYS_CHANNEL8)
      static Event& assign_generator(gen8::generator_t generator) { set_generator((gen::generator_t)generator, 8); }
    #endif
    #if defined(EVSYS_CHANNEL9)
      static Event& assign_generator(gen9::generator_t generator) { set_generator((gen::generator_t)generator, 9); }
    #endif

Event.cpp

Event& Event::assign_generator_pin(uint8_t pin_number) {
  uint8_t port = digitalPinToPort(pin_number);
  uint8_t port_pin = digitalPinToBitPosition(pin_number);
  return Event::assign_generator_pin(port, port_pin)
}

Event& Event::assign_generator_pin(uint8_t port, uint8_t port_pin) {
  if (port != NOT_A_PIN && port_pin != NOT_A_PIN) {
    #if !defined(MEGATINYCORE)
      uint8_t gen = 0x40 | (port & 0x01) << 3 | port_pin;
      if (port == PA || port == PB) {
        if (Event0.generator_type == gen::disable || Event0.generator_type == gen) {
          Event0.generator_type = gen;
          return Event0;
        }
        else if (Event1.generator_type == gen::disable || Event1.generator_type == gen) {
          Event1.generator_type = gen;
          return Event1;
        }
      }
      else if (port == PC || port == PD) {
        if (Event2.generator_type == gen::disable || Event2.generator_type == gen) {
          Event2.generator_type = gen;
          return Event2;
        }
        else if (Event3.generator_type == gen::disable || Event3.generator_type == gen) {
          Event3.generator_type = gen;
          return Event3;
        }
      }
      else if (port == PE || port == PF) {
        if (Event4.generator_type == gen::disable || Event4.generator_type == gen) {
          Event4.generator_type = gen;
          return Event4;
        }
        else if (Event5.generator_type == gen::disable || Event5.generator_type == gen) {
          Event5.generator_type = gen;
          return Event5;
        }
      }
      #if defined(Dx_64_PINS)
      else if (port == PG) {
        if (Event6.generator_type == gen::disable || Event6.generator_type == gen) {
          Event6.generator_type = gen;
          return Event6;
        }
        else if (Event7.generator_type == gen::disable || Event7.generator_type == gen) {
          Event7.generator_type = gen;
          return Event7;
        }
      }
      #endif
    }
    return Event_empty;
  #elif MEGATINYCORE_SERIES == 2
    // TODO: Much like above, except it's PA/PB, PB/PC, PC/PA
  #else // Oh no, it's a 0/1-series!
    // TODO: PA at one set of offsets, PB at another, then async channels have PA, PB or PC at yet a third offset
    // and 0-series skips odd numbered channels and hence can't do events from PB at all.
  #endif
}

Event& Event::assign_generator(gen::generator_t gen, uint8_t ch = 255) {
  #if !defined(MEGATINYCORE) || MEGATINYCORE_SERIES == 2
    if (ch != 255) { // this means it can only be the divided, pins, or disable,
      if (gen == 0) {
        // What the hell are they doing asking for disabled with a specific channel's constant?!
        // They're doing it wrong, whatever it is. If you want a disabled channel, specify gen::disable not genN::disable
        // One could argue we should return the EventN if it's disabled.
        // But if they know the event channel number, and want to see if it's disabled, there are other ways to do that, and
        // if they just want any unused channel, passing gen::disable will do that. 
        // I'm more inclined to just always return a failure because any interpretation of that argument has other ways of 
        /// achieving it that do not require their code know anything more than it has to to write that. 
        // This is "Put this generator on an event channel (unless it already is on one) and give me a reference to that channel 
        // either way" - I considered return of Event_empty for gen::disable too. But at least that's not ambiguous, 
        return Event_empty;
      }
      if (gen > 0x10) {
        // pin generator - couldn't the asshole have called assign_generator_pin()?
        // There is no standard way to get digital pin number from port and bit.
        // But in assign_generator_pin(uint8_t) we have to convert to port and bit anyway
        // so I split that up, calculate the port and bit position here, and call that.
        #if !defined(MEGATINYCORE)
          uint8_t port = ch & 0xFE + (gen & 0x08 ? 1 : 0);
          uint8_t port_pin = gen & 0x07;
        #else MEGATINYCORE_SERIES == 2 //2-series has PA/PB, PB/PC, PC/PA - it wraps around
          uint8_t port = ch >> 1 + (gen & 0x08 ? 1 : 0);
          if (port == 3) port = 0;
          uint8_t port_pin = gen & 0x07;
        #endif
        return Event::assign_generator_pin(port,port_pin);
      } else {
        // This could only be an RTC channel
        if (ch & 1) {
          #if defined(EVSYS_CHANNEL9)
            if (Event9.generator_type == gen::disable || Event9.generator_type == gen) {
              Event9.generator_type = gen;
              return Event9;
            } else
          #endif
          #if defined(EVSYS_CHANNEL7)
            if (Event7.generator_type == gen::disable || Event7.generator_type == gen) {
              Event7.generator_type = gen;
              return Event7;
            } else
          #endif
          #if defined(EVSYS_CHANNEL5)
            if (Event5.generator_type == gen::disable || Event5.generator_type == gen) {
              Event5.generator_type = gen;
              return Event5;
            } else
          #endif
          #if defined(EVSYS_CHANNEL3)
            if (Event3.generator_type == gen::disable || Event3.generator_type == gen) {
              Event3.generator_type = gen;
              return Event3;
            } else
          #endif
          #if defined(EVSYS_CHANNEL1)
            if (Event1.generator_type == gen::disable || Event1.generator_type == gen) {
              Event1.generator_type = gen;
              return Event1;
            } else
          #endif
            return Event_empty;
        } else {
          #if defined(EVSYS_CHANNEL8)
            if (Event8.generator_type == gen::disable || Event8.generator_type == gen) {
              Event8.generator_type = gen;
              return Event8;
            } else
          #endif
          #if defined(EVSYS_CHANNEL6)
            if (Event6.generator_type == gen::disable || Event6.generator_type == gen) {
              Event6.generator_type = gen;
              return Event6;
            } else
          #endif
          #if defined(EVSYS_CHANNEL4)
            if (Event4.generator_type == gen::disable || Event4.generator_type == gen) {
              Event4.generator_type = gen;
              return Event4;
            } else
          #endif
          #if defined(EVSYS_CHANNEL2)
            if (Event2.generator_type == gen::disable || Event2.generator_type == gen) {
              Event2.generator_type = gen;
              return Event2;
            } else
          #endif
          #if defined(EVSYS_CHANNEL0)
            if (Event0.generator_type == gen::disable || Event0.generator_type == gen) {
              Event0.generator_type = gen;
              return Event0;
            } else
          #endif
            return Event_empty;
        }
      }
    } else { // otherwise it could be on any channel, so check if it's already live on a channel first
      Event& chan=Event::get_event_channel(gen);
      if (chan.get_channel_number() != 255) { // is this right?
        return chan;
      } else {
        #if defined(EVSYS_CHANNEL9)
          if (Event9.generator_type == gen::disable) {
            Event9.generator_type = gen;
            return Event9;
          } else
        #endif
        #if defined(EVSYS_CHANNEL8)
          if (Event8.generator_type == gen::disable) {
            Event8.generator_type = gen;
            return Event8;
          } else
        #endif
        #if defined(EVSYS_CHANNEL7)
          if (Event7.generator_type == gen::disable) {
            Event7.generator_type = gen;
            return Event7;
          } else
        #endif
        #if defined(EVSYS_CHANNEL6)
          if (Event6.generator_type == gen::disable) {
            Event6.generator_type = gen;
            return Event6;
          } else
        #endif
        #if defined(EVSYS_CHANNEL5)
          if (Event5.generator_type == gen::disable) {
            Event5.generator_type = gen;
            return Event5;
          } else
        #endif
        #if defined(EVSYS_CHANNEL4)
          if (Event4.generator_type == gen::disable) {
            Event4.generator_type = gen;
            return Event4;
          } else
        #endif
        #if defined(EVSYS_CHANNEL3)
          if (Event3.generator_type == gen::disable) {
            Event3.generator_type = gen;
            return Event3;
          } else
        #endif
        #if defined(EVSYS_CHANNEL2)
          if (Event2.generator_type == gen::disable) {
            Event2.generator_type = gen;
            return Event2;
          } else
        #endif
        #if defined(EVSYS_CHANNEL1)
          if (Event1.generator_type == gen::disable) {
            Event1.generator_type = gen;
            return Event1;
          } else
        #endif
        #if defined(EVSYS_CHANNEL0)
          if (Event0.generator_type == gen::disable) {
            Event0.generator_type = gen;
            return Event0;
          } else
        #endif
          return chan; // if we're on this branch, we know chan is Event_empty.
      }
    }
  #else
    // It's a 0/1-series tinyAVR. The event system is a raging shitshow here, and everything has to be different. At least there are only 6 channels.
    // or if the poor user is torturing themselves and using a 0-series, 3
    // At least the RTC part is relatively less painful for these parts - 0-series doesn't have the option, and 1-series only has one option.
    // Actually, the parts are so severely constrained a lot of these things simplify dramatically, but it's still a completely separate implementation
  #endif
}

Code is untested currently, I hope to have time to try it out tonight but this is along the lines that I'm thinking. Does this approach seem sound?

I'm not terribly confident in my understanding of classes (It's true: I'm just not a classy guy)

There's still the issue of how to find the user/generator numbers for peripherals. There aren't that many that need to be handled, though. I'm mostly concerned with the ones that people are likely to be using pointers/references to.

As I think I've said, I want a Logic::get_unused_

I think you;ve covered the rest of the bases, in terms of providing an API to permit portable code, so now someone could write a library to do, say, input capture has a way to say "Give me a channel that uses this generator" and I can then assign a user to it without needing a bunch of #ifdefs to cover different devices with different channel layouts.

Implementing assign_generator and assign_generator_pin for tinyAVR 0/1-series will be.... unpleasant. but so is doing anything with the awful "public beta" version of EVSYS that those parts got. (At least they came to their senses before releasing anything else like that.) But that's what libraries included with a core are for.

Assuming above code works and you think the approach is sane, after I add Dx-series support to set_user_pin and tinyAVR support to the above and port the 0/1-series support from the soft-event, it would be good if we could get to one copy of the library with support for everything in one set of .h and .cpp files

Regarding linting and formatting

There is no way a library like this will ever pass any linter at the indentation level. The formatting of the enums it mine wants is unreadable (really, there's no "great" option there, only "less bad" ones. and places where if/else if structures are chopped into pieces and reassembled by the preprocessor understandably confuse it - I turn off the indentation check for files where there is no hope of making it pass while remaining readable.

I don't have a code formatter either. I have a linter that yells at me and marks the code as failing if it doesn't like the formatting.
I don't have anything that I can pass code through to output equivalent code that passes the linter.

The linter tells me where there are problems, which I then need to either fix manually or write regex substitution patterns to fix in bulk. I can turn off the indentation check on a per file or part of file basis (and do for files where I know there is no way for a file to pass the linter without failing the "human eyeball" test ("Does the formatting make the code more or less readable?")) That's why there are so many commits in my repos to fix formatting to conform to the linter's demands - I submit code, and then look at the github action results to see if it passes lint, and then if it fails, fix or disable linting there as appropriate, check in the changes and see if I get a green checkmark or another red X.

The point of linting for code style is to make the style readable, consistent, and pretty - in that order. The automated check, astyle, never handles nested preprocessor directives well, (that's why it isn't run on my cores themselves, only libraries and examples - All code styling on the core files is 100% manual) but I don't know of a better linter, and I really do want the examples, and where possible, the libraries to be consistently formatted. It enforces putting spaces between operators and their operands (which I often slip up on), and having curly-braces for if/else statements formatted the Right Way, and it gets indentation right in most examples and libraries. When consistent formatting would get in the way of readability (generally limited to indentation, in cases where we're using preprocessor directives heavily and/or to split up if/else if/else structures like we do here, plus a few weird cases like inline assembly and tables like the gamma table in tinyNeoPixel), readability takes priority and I override/disable the linting, though I try to keep it as close to a consistent style as possible.

@aikopras
Copy link
Author

Assuming above code works and you think the approach is sane, after I add Dx-series support to set_user_pin and tinyAVR support to the above and port the 0/1-series support from the soft-event, it would be good if we could get to one copy of the library with support for everything in one set of .h and .cpp files

I would be happy to test the library (or better:those parts of the library that I'm using) on both MegaCoreX (4808 and 4809) as well as DxCore (AVR128DA48).

Regarding portability: the Timer Event users on the AVR DA and DB series have a slightly different name: instead of tcb* it is now tcb*_capt, or tcb*_cnt.
tcb*_capt is equivalent with the MegaCoreX tcb*, so having the same name in both libraries would be nice. tcb*_cnt is new, and only on DA and DB.

@SpenceKonde
Copy link
Contributor

Agree on the tcb naming. That's not hard to add an alias for...

As an aside, the CNT user isn't just on the Dx - 2-series tinys have it, EA-series is pretty much a sure bet too. It looks like they just xerox the same design across all the parts designed after they got it working - unless something really does cost more to implement, (hence why things like the DAC and TCD get dropped, while the timer upgrades don't). (the exception to this is how the EA CPU itself seems to be a downgrade >.> )

@MCUdude
Copy link
Owner

MCUdude commented Oct 27, 2021

  1. I think set_generator_pin() is not a good name - the static set_generator_pin does something fundamentally different from the non-static set_something() member functions. I propose "assign_generator_pin" to capture that it does more than just configuring that pin, it also chooses a usable channel.
  2. assign_generator_pin should be able to return a currently configured channel if it's set to the requested pin.
  3. The last part is to take a generator instead of a pin number and do the same thing: assign_generator()

@SpenceKonde I do agree with you on 1. and 2.

But can you please explain what you're trying to accomplish in 3.? What does it do and why do we need it? set_generator() / assign_generator() already takes a generator as a parameter, and you use to object itself to select which channel to "connect" the generator to.

MCUdude added a commit that referenced this issue Oct 27, 2021
Minor formatting and typo fixes. Rename set_generator and set_generator_pin to assign_generator and assign_generator_pin. #126 related
@MCUdude
Copy link
Owner

MCUdude commented Nov 28, 2021

@SpenceKonde I'm still waiting for your response

@aikopras
Copy link
Author

aikopras commented Dec 4, 2021

Whereas the new library works great on MegaCoreX, I can't get the DxCore version running. On DxCore I had to restore to the event.h and event.cpp files from March, to get at least some event functionality running. The newer versions (including from November 12) no longer seem to compile.

@SpenceKonde: Although the issue doesn't relate to MegaCoreX but to DxCore, I post it here to keep the discussion at 1 place. I hope that is OK.

@SpenceKonde
Copy link
Contributor

SpenceKonde commented Dec 4, 2021 via email

@SpenceKonde
Copy link
Contributor

SpenceKonde commented Dec 6, 2021

And now, at 2:50 AM on sunday night, I've finally gotten the last of the serial changes in.... this is the last thing to get settled for DxCore 1.4.0 and 2.5.0, which I want to make available before all of my time is consumed with an unplanned forced move to another apartment.

@aikopras can you be more specific about the problems on DxCore??? They are passing the automated tests...

@MCUdude
Copy link
Owner

MCUdude commented Dec 6, 2021

@SpenceKonde are you happy with the current state of the Event library, functionality-wise? If yes, I'll probably just create a few more examples and merge the whole thing into master.

@SpenceKonde
Copy link
Contributor

I just finished implementing tinyAVR 0/1-series version of assign_generator, assign_generator pin, and I think some other place where I had a TODO comment, and added the DD-series defines and fixed a few copy-paste errors, and it now compiles on Dx and. I have no idea if it actually works though.

https://github.com/SpenceKonde/DxCore/blob/master/megaavr/libraries/Event/src/Event.cpp

@SpenceKonde
Copy link
Contributor

Updated to fix some issues that came up on megaTinyCore (note that it requires latest version of megaTinyCore for that to work)

@SpenceKonde
Copy link
Contributor

This is the sort of thing I was thinking about for other peripherals....

gen::generator_t Event::gen_from_peripheral(&TCB_t timer, uint8_t event_type) {
  uint8_t addr = (uint8_t)(uint16_t) &timer;
  #if !defined(MEGATINYCORE) || MEGATINYCORE_SERIES == 2
    addr >> 3; //now it's 0, 2, 4, 6, or 8. 
    #if !defined(DXCORE)
      if (eventType > 1) 
    #else 
      if (event_type)
    #endif
      addr = 0;
    else {
      addr+= event_type & 0x01; // 0x00 = capt, 0x01 = ovf
      addr += 0xA0;
    }
  #else 
    /* megatinycore */
  #endif
  return (gen::generator_t) addr;
}
user::user_t Event::user_from_peripheral(&TCB_t timer, uint8_t user_type) {
  uint8_t addr = (uint8_t)(uint16_t) &timer;
  #if !defined(MEGATINYCORE)
    #if defined(DXCORE)
      addr >> 3; //now it's 0, 2, 4, 6, or 8. 
      if (eventType > 1) 
        addr = 0; // invalid user requested.
      else {
        addr += event_type & 0x01; // 0x00 = capt, 0x01 = ovf
        #if defined(__AVR_DA__)
          addr += 0x1F;
        #else
          addr += 0x1E
        #endif
      }
    #else 
      if (event_type)
        addr = 0;
      else {
        addr >> 4; //now it's 0, 1, 2, 3, or 4. 
        addr += 0x13; 
      }
    #endif
  #else 
    // handle tinyavr
  #endif
  return (user::user_t) addr;
}

and you shoudl be able to cover different kinds of peripherals like this, right?

    static gen::generator_t gen_from_peripheral(&TCB_t timer, uint8_t event_type);
    static gen::generator_t gen_from_peripheral(&AC_t comp, uint8_t event_type);
    static gen::generator_t gen_from_peripheral(&TCA_t timer, uint8_t event_type);
    static gen::generator_t gen_from_peripheral(&ZCD_t zerocross, uint8_t event_type);
    static gen::generator_t gen_from_peripheral(&CCL_t logic, uint8_t event_type);

@SpenceKonde
Copy link
Contributor

Okay, I've got versions in that pass compile-test checked in for DxCore and megaTinyCore now for what I think are the important ones.

@MCUdude
Copy link
Owner

MCUdude commented Dec 6, 2021

I'll review your addition and merge the changes into MegaCoreX, and do some chip-spesific changes if necessary. You should probably add some example sketches and add them to the README as well.

@SpenceKonde
Copy link
Contributor

Unless you object, I propose bumping the version number in library.properties to 1.2.0.

@MCUdude
Copy link
Owner

MCUdude commented Dec 7, 2021

Sounds good! I'll spend some time this evening. BTW why pass the structs as pointers and not references?
If a reference is used, the user can use Event::gen_from_peripheral(TCB3, some_event_type); instead of Event::gen_from_peripheral(&TCB3, some_event_type);.

BTW would you like to explain what the event_type and user_type parameters are and how the user should know what to insert? To be honest, it has been a while since I've worked with the Event system implementation, and it's something that easily slips off your mind when you don't use it regularly because it tends to be a bit tricky to wrap your head around.

@SpenceKonde
Copy link
Contributor

SpenceKonde commented Dec 7, 2021

I've just committed what I hope is final (not sure I'll have time for examples)

I updated the API reference in the docs though.

I tried the references first -but it wouldn't work. It said I wasn't allowed to overload them, and didn't seem to be able to tell the types apart :-( If you're able to make it work, show me how, I'd have preferred that, if it would have compiled.

event_type and user_type are just for peripherals that have more than one available - like how a TCA can generate something 5 different events and on Dx and 2-series, consume two different kinds., and the CCLs are all one peripheral, There's now a table in the documentation too.

I noticed my note in the docs about the long_soft_event; It was trivial enough to implement that I did that too - it's not particularly complicated as far as asm goes, and I wanted to make sure I had the option to do at least 2 clocks of continuously asserted software event, since you want to be able to force an event through the filter on a CCL, and you can't do that with a normal softevent, no matter what, because the compiler would render it as sts, a 2-word 2-clock instruction. Even if you got 2 in a row, the soft_event would be over halfway to the next write. I generally wanted the option be able to hold a soft-event for at least long enough for it to not just make it through the filter, but still be asserted state after it comes out the other side of the filter

I also added the USART to the list of thing that you can user_from_peripheral() , just for getting the event input(I'm pretty sure you can use that to remap any event channel (and hence any pin) to a serial port's RX. I didn't have a chance to try it out yet (was too busy with the rest of the serial stuff. (The serial implementation I'm using now supports half duplex UART, RS485 mode, and (via a tiny bit of manual effort, or use of a helper macro) USART (clocked) or MSPI mode.), and I made up for the size penalty by reimplementing the ISRs in assembly to get rid of the duplicated prologue and epilogues that save and restore registers that the function doesn't use, same trick I used on attachInterrupt).

@aikopras
Copy link
Author

aikopras commented Dec 7, 2021

Great, Thanks :-)

There are a few minor issues:

  1. missing semicolon at end of sentence in event.cpp (DxCore):
    Line 925: _long_soft_event(channel_number, length)
    Line 1140: addr += 0x1B // DA or mega0

  2. Missing aliases
    The following works on DxCore, but not MegaCoreX
    myEvent.set_user(user::tcb1_capt); // DxCore

The following works on MegaCoreX, but not DxCore
myEvent.set_user(user::tcb1); // MegaCoreX

A solution could be to add some tcb related aliases to MegaCoreX.

If I find more, I let you know. Next to compiling it also actually works ;-) (at least for my libraries)

@MCUdude
Copy link
Owner

MCUdude commented Dec 10, 2021

@SpenceKonde I'm currently looking at your user_from_peripheral() and gen_from_peripheral(), and I don't get why you have made them so complicated. It's pretty much impossible to follow what all the hex values and bit shifts are for. Why couldn't we do something like this instead? Why all the bit shifts and stuff when we're going to return a known gen:: or user:: based purely on the device type and input parameters?

gen::generator_t Event::gen_from_peripheral(AC_t& comp)
{  
  #if defined(__AVR_ATmegax08__) || defined(__AVR_ATmegax09__)
    return gen::ac0_out;
  #elif defined(DXCORE)
    if(&comp == &AC0)
      return gen::ac0_out;
    else if(&comp == &AC1)
      return gen::ac1_out;
    else if(&comp == &AC2)
      return gen::ac2_out;
  #endif  
}

@MCUdude
Copy link
Owner

MCUdude commented Dec 11, 2021

Well, because I must have been doing something wrong when I was trying to do it with references, and once I made it work with pointers, I didn't go backl to references and try to figure out why it wasn't working for me., and I just didn't think of testing them directly.

I think in the case of the TCB'sthe sort of trickery I use could reduce code size a tiny bit, but I'm not certain, and for the parts that care (which only have 2 TCBs at most), it won't save space.

But yeah, that alternative never crossed my mind. If that works and is the same or smaller compiled, than by all means!

If that works, then the best implementation I think would be this - no part-specific knowledge is needed whatsoever. All it needs to know is if AC0, AC1 and AC2 exist, and the only assumption it makes is that for ACn, n < 3 and there is an ACn-1 and so on.

gen::generator_t Event::gen_from_peripheral(AC_t& comp)
{
#if defined(AC0)
  #if defined(AC1) ||  defined(AC2)
    if (&comp == &AC0) 
  #endif
    return gen::ac0_out;
  else 
  #if defined(AC1)
    if (&comp == &AC1) 
  #endif
    return gen::ac1_out;
  else 
  #if defined(AC2)
    if(&comp == &AC2)
      return gen::ac2_out;
  #endif
  else 
#endif
    return (gen::generator_t) -1; //is there a better way to indicate something that will be rejected by anything that takes a generayor?
}

Yes, we should probably use this implementation for all gen_from_peripheral and user_from_peripheral. Another benefit is that the code is much easier to read and understand.

I've also modified your assign_generator() implementation too. Why even bother trying to deal with Arduino pin numbers and channel-specific events when the function takes a gen::generator_t enum as a parameter anyways? My implementation only has one parameter and is way less complex, and still does what users expect it to do.
https://github.com/MCUdude/MegaCoreX/blob/New-Event-library/megaavr/libraries/Event/src/Event.cpp#L386

I've also done some changes to get_generator_channel(). Now gen::/genN:: enums and Arduino pins are handled differently. Previously it was impossible to get a reference to the channel object of an Arduino pin was set as the generator.

@SpenceKonde
Copy link
Contributor

Agree - if that works, that's better than my approach (though the CCL won't change much)

I don't know what you mean by "when the function takes a gen::generator_t enum as a parameter anyways?"

The pins aren't such a big deal; there's a proper function for that. How do you handle an "assign" for an RTC channel now though? Should I document that we do not support automatic discovery of available channels for the RTC generators?

Okay, I'm going to adapt my gen/user from peripheral functions and merge your functions back into my copy, (you realize that my intent was that the files I was adding would be portable to megaAVR 0 as well as DxCore and megaTinyCore, right? It would be lovely if we could have a single copy of the evvent library that worked everywhere; not only is it a single copy of the library being maintained, but it enforces a uniform API - which is where most of the payoff is reaped.

The version that I'm reconstituting will have the aliases that @aikopras requested too.

@MCUdude
Copy link
Owner

MCUdude commented Dec 11, 2021

I don't know what you mean by "when the function takes a gen::generator_t enum as a parameter anyways?"

You can't pass gen0::rtc_div8192 or anything else other than gen:: to the assign_generator().

If you try to pass something else, Event::assign_generator(gen0::rtc_div8192);, the compiler won't let you without casting. That's why I didn't care about Arduino pins or the RTC. It's only meant for generators that can be assigned to any channel, not specific ones like the RTC and digital pins requires.

Okay, I'm going to adapt my gen/user from peripheral functions and merge your functions back into my copy, (you realize that my intent was that the files I was adding would be portable to megaAVR 0 as well as DxCore and megaTinyCore, right? It would be lovely if we could have a single copy of the event library that worked everywhere; not only is it a single copy of the library being maintained, but it enforces a uniform API - which is where most of the payoff is reaped.

It's nice to only have one copy of the library, but you and I have vastly different coding styles, and I'm not really that excited about dense and somewhat cryptic code.

  • I would obviously prefer my own coding style. It's not as dense, but I feel it's more readable and easier to follow. I am mainly thinking about brackets on separate lines and a nicely formatted Doxygen-styled summary above the function so that "modern" text editors can give the user a nice preview.
  • Macros kills code readability. It's just a fact. I do understand that it has to be there, but we should strive to make macros as easy to follow as possible. #if (!defined(MEGATINYCORE) || MEGATINYCORE_SERIES != 2) is a good example of a macro that is difficult to follow and makes your brain itch. Not MEGATINYCORE or not the tiny-2 series? What is this even supposed to mean? IMO, code readability is a must, even if it makes for more verbose code.

@SpenceKonde
Copy link
Contributor

Well, the pins are easy enough to handle. It is awkward about the RTC though.

I strongly dislike the opening brackets being on their own line. It's sort of strange to me that while I don't like opening brackets getting their own line, I only very rarely omit the brackets altogether (maybe the fact that it costs 0-1 line instead of 1-2 lines is part of that?), and when I do, it's because the conditional code is something particularly clear - short simple test, the code that is conditionally run is something that obviously is being run conditionally like a return statement when checking inputs for errors, where it's hard to not understand what is going on.

And yeah, that macro is weird. And it's repeated all over the place in my code, yup. Why? Because the tinyAVR 2-series has the event system of the Dx, with only minor differences. Almost all code can be shared. I could define something... clearer than that test in one place, (which would just be defined as that but in only one place, instead of scattered everywhere.

The tiny0/1 on the other hand.... that's a total shitshow. Look at the struct:

typedef struct EVSYS_struct
{
    register8_t ASYNCSTROBE;  /* Asynchronous Channel Strobe */
    register8_t SYNCSTROBE;  /* Synchronous Channel Strobe */
    register8_t ASYNCCH0;  /* Asynchronous Channel 0 Generator Selection */
    register8_t ASYNCCH1;  /* Asynchronous Channel 1 Generator Selection */
    register8_t ASYNCCH2;  /* Asynchronous Channel 2 Generator Selection */
    register8_t ASYNCCH3;  /* Asynchronous Channel 3 Generator Selection */
    register8_t reserved_1[4];
    register8_t SYNCCH0;  /* Synchronous Channel 0 Generator Selection */
    register8_t SYNCCH1;  /* Synchronous Channel 1 Generator Selection */
    register8_t reserved_2[6];
    register8_t ASYNCUSER0;  /* Asynchronous User Ch 0 Input Selection - TCB0 */
    register8_t ASYNCUSER1;  /* Asynchronous User Ch 1 Input Selection - ADC0 */
    register8_t ASYNCUSER2;  /* Asynchronous User Ch 2 Input Selection - CCL LUT0 Event 0 */
    register8_t ASYNCUSER3;  /* Asynchronous User Ch 3 Input Selection - CCL LUT1 Event 0 */
    register8_t ASYNCUSER4;  /* Asynchronous User Ch 4 Input Selection - CCL LUT0 Event 1 */
    register8_t ASYNCUSER5;  /* Asynchronous User Ch 5 Input Selection - CCL LUT1 Event 1 */
    register8_t ASYNCUSER6;  /* Asynchronous User Ch 6 Input Selection - TCD0 Event 0 */
    register8_t ASYNCUSER7;  /* Asynchronous User Ch 7 Input Selection - TCD0 Event 1 */
    register8_t ASYNCUSER8;  /* Asynchronous User Ch 8 Input Selection - Event Out 0 */
    register8_t ASYNCUSER9;  /* Asynchronous User Ch 9 Input Selection - Event Out 1 */
    register8_t ASYNCUSER10;  /* Asynchronous User Ch 10 Input Selection - Event Out 2 */
    register8_t ASYNCUSER11;  /* Asynchronous User Ch 11 Input Selection - TCB1 */
    register8_t ASYNCUSER12;  /* Asynchronous User Ch 12 Input Selection - ADC1 */
    register8_t reserved_3[3];
    register8_t SYNCUSER0;  /* Synchronous User Ch 0 - TCA0 */
    register8_t SYNCUSER1;  /* Synchronous User Ch 1 - USART0 */
    register8_t reserved_4[28];
} EVSYS_t;

megaTinyCore already has a bunch of #defines to rename them to match the rest of the world, but it's still wacky in lots of ways.

But yeah - I'm writing for three different targets - tinyAVR 0/1, tinyAVR 2, and Dx-series. though tinyAVR2 and Dx-series are very nearly identical. (and mega 0-series is almost identical to Dx afaik).

On the topic of macros and readability - since you have the PIN_Pxn defines in your core too, why don't we just check for those instead of part numbers? (eg, in the pin user section). Seems clearer than checking pincounts.

@MCUdude
Copy link
Owner

MCUdude commented Dec 11, 2021

I strongly dislike the opening brackets being on their own line. It's sort of strange to me that while I don't like opening brackets getting their own line, I only very rarely omit the brackets altogether (maybe the fact that it costs 0-1 line instead of 1-2 lines is part of that?), and when I do, it's because the conditional code is something particularly clear - short simple test, the code that is conditionally run is something that obviously is being run conditionally like a return statement when checking inputs for errors, where it's hard to not understand what is going on.

I realize now that the Logic library is already formatted in your favor, so OK, let's keep your formatting style, even though my personal preferences differ.

And yeah, that macro is weird. And it's repeated all over the place in my code, yup. Why? Because the tinyAVR 2-series has the event system of the Dx, with only minor differences. Almost all code can be shared. I could define something... clearer than that test in one place, (which would just be defined as that but in only one place, instead of scattered everywhere.

On the topic of macros and readability - since you have the PIN_Pxn defines in your core too, why don't we just check for those instead of part numbers? (eg, in the pin user section). Seems clearer than checking pincounts.

Good idea! Could something like this work?

#if defined(MEGACOREX) || defined(DXCORE)
// Features present on all generator channels
namespace gen {
  enum generator_t : uint8_t {
    disable       = 0x00,
    off           = 0x00,
    updi_synch    = 0x01,
    rtc_ovf       = 0x06,
    rtc_cmp       = 0x07,
    ccl0_out      = 0x10,
    ccl1_out      = 0x11,
    ccl2_out      = 0x12,
    ccl3_out      = 0x13,
    ac0_out       = 0x20,
#if defined(AC1)
    ac1_out       = 0x21,
#endif
#if defined(AC2)
    ac2_out       = 0x22,
#endif
    adc0_ready    = 0x24,
    usart0_xck    = 0x60,
    usart1_xck    = 0x61,
    usart2_xck    = 0x62,
#if defined(USART3)
    usart3_xck    = 0x63,
#endif
    spi0_sck      = 0x68,
    tca0_ovf_lunf = 0x80,
    tca0_hunf     = 0x81,
    tca0_cmp0     = 0x84,
    tca0_cmp1     = 0x85,
    tca0_cmp2     = 0x86,
    tcb0_capt     = 0xA0,
    tcb0          = 0xA0,
    tcb1_capt     = 0xA2,
    tcb1          = 0xA2,
    tcb2_capt     = 0xA4,
    tcb2          = 0xA4,
#if defined(TCB3)
    tcb3_capt     = 0xA6,
    tcb3          = 0xA6,
#endif
#if defined(TCB4)
    tcb4_capt      = 0xA8,
    tcb4           = 0xA8,
#endif
#if defined(__AVR_DA__) || defined(__AVR_DB__)
    ccl4_out      = 0x14,
    ccl5_out      = 0x15,
    zcd0_out      = 0x30,
    zcd1_out      = 0x31,
#if defined(ZCD2)
    zcd2_out      = 0x32,
#endif
#if defined(USART4)
    usart4_xck    = 0x64,
#endif
#if defined(USART5)
    usart5_xck    = 0x65,
#endif
    spi1_sck      = 0x69,
#if defined(TCA1)
    tca1_ovf_lunf = 0x88,
    tca1_hunf     = 0x89,
    tca1_cmp0     = 0x8C,
    tca1_cmp1     = 0x8D,
    tca1_cmp2     = 0x8E,
#endif
#endif // defined(__AVR_DA__) || defined(__AVR_DB__)
#if defined(DXCORE)
// These are present on every modern part released since the 0/1-series and will probably continue to be, so check
    tcb0_ovf      = 0xA1,
    tcb1_ovf      = 0xA3,
#if defined(TCB2)
    tcb2_ovf      = 0xA5,
#endif
#if defined(TCB3)
    tcb3_ovf      = 0xA7,
#endif
#if defined(TCB4)
    tcb4_ovf      = 0xA9,
#endif
#endif
#if defined(TCD0)
    tcd0_cmpbclr  = 0xB0,
    tcd0_cmpaset  = 0xB1,
    tcd0_cmpbset  = 0xB2,
    tcd0_progev   = 0xB3,
#endif
#if defined(MVIO)
    mvio_ok       = 0x05,
#endif
#if defined(ZCD3)
    zcd3_out      = 0x30, // The ZCD is numbered differently, I think just because it's on different pins. Still has the same event channel
#endif
#if defined(__AVR_DB__)
    opamp0_ready  = 0x34,
    opamp1_ready  = 0x35,
#if defined (OPAMP2)
    opamp2_ready  = 0x36,
#endif
#endif // defined(__AVR_DB__)
  };
};

// Features unique to event generator channel 0
#if defined(EVSYS_CHANNEL0)
namespace gen0 {
  enum generator_t : uint8_t {
    disable     = 0x00,
    off         = 0x00,
    rtc_div8192 = 0x08,
    rtc_div4096 = 0x09,
    rtc_div2048 = 0x0A,
    rtc_div1024 = 0x0B,
    pin_pa0     = 0x40,
    pin_pa1     = 0x41,
#if !defined(DXCORE) || defined(PIN_PA2)
    pin_pa2     = 0x42,
    pin_pa3     = 0x43,
    pin_pa4     = 0x44,
    pin_pa5     = 0x45,
    pin_pa6     = 0x46,
    pin_pa7     = 0x47,
#endif
#if defined(PIN_PB0)
    pin_pb0     = 0x48,
    pin_pb1     = 0x49,
    pin_pb2     = 0x4A,
    pin_pb3     = 0x4B,
    pin_pb4     = 0x4C,
    pin_pb5     = 0x4F,
#endif
#if defined(PIN_PB6)
    pin_pb6     = 0x4E,
    pin_pb7     = 0x4F,
#endif
  };
};
#endif

// Features unique to event generator channel 1
#if defined(EVSYS_CHANNEL1)
namespace gen1 {
  enum generator_t : uint8_t {
    disable     = 0x00,
    off         = 0x00,
    rtc_div512  = 0x08,
    rtc_div256  = 0x09,
    rtc_div128  = 0x0A,
    rtc_div64   = 0x0B,
    pin_pa0     = 0x40,
    pin_pa1     = 0x41,
#if defined(PIN_PA2)
    pin_pa2     = 0x42,
    pin_pa3     = 0x43,
    pin_pa4     = 0x44,
    pin_pa5     = 0x45,
    pin_pa6     = 0x46,
    pin_pa7     = 0x47,
#endif
#if defined(PIN_PB0)
    pin_pb0     = 0x48,
    pin_pb1     = 0x49,
    pin_pb2     = 0x4A,
    pin_pb3     = 0x4B,
    pin_pb4     = 0x4C,
    pin_pb5     = 0x4F,
#endif
#if defined(PIN_PB6)
    pin_pb6     = 0x4E,
    pin_pb7     = 0x4F,
#endif
  };
};
#endif

// Features unique to event generator channel 2
#if defined(EVSYS_CHANNEL2)
namespace gen2 {
  enum generator_t : uint8_t {
    disable     = 0x00,
    off         = 0x00,
    rtc_div8192 = 0x08,
    rtc_div4096 = 0x09,
    rtc_div2048 = 0x0A,
    rtc_div1024 = 0x0B,
#if !defined(DD_14_PINS)  // Can't just check PIN_PC0 - the 0-pin of any port with any pins present is always defined by DxCore.
                          // I considered both ways, but there are to many reasons we need to have the 0-pin defined.
    pin_pc0     = 0x40,
#endif
    pin_pc1     = 0x41,
    pin_pc2     = 0x42,
    pin_pc3     = 0x43,
#if defined(PIN_PC4)
    pin_pc4     = 0x44,
    pin_pc5     = 0x45,
    pin_pc6     = 0x46,
    pin_pc7     = 0x47,
#endif
#if defined(PIN_PD1) // See above for note on PIN_PD0 and why we can't test that, this even impacts DBs.
#if !defined(MVIO) || defined(__AVR_ATmegax09__) || defined(Dx_48_PINS) || defined(Dx_64_PINS)
    pin_pd0     = 0x48,
#endif
    pin_pd1     = 0x49,
    pin_pd2     = 0x4A,
    pin_pd3     = 0x4B,
#endif
    pin_pd4     = 0x4C,
    pin_pd5     = 0x4F,
    pin_pd6     = 0x4E,
    pin_pd7     = 0x4F,
  };
};
#endif

// Features unique to event generator channel 3
#if defined(EVSYS_CHANNEL3)
namespace gen3 {
  enum generator_t : uint8_t {
    disable     = 0x00,
    off         = 0x00,
    rtc_div512  = 0x08,
    rtc_div256  = 0x09,
    rtc_div128  = 0x0A,
    rtc_div64   = 0x0B,
#if !defined(DD_14_PINS)
    pin_pc0     = 0x40,
#endif
    pin_pc1     = 0x41,
    pin_pc2     = 0x42,
    pin_pc3     = 0x43,
#if defined(PIN_PC4)
    pin_pc4     = 0x44,
    pin_pc5     = 0x45,
    pin_pc6     = 0x46,
    pin_pc7     = 0x47,
#endif
#if defined(PIN_PD1) // See above for note on PIN_PD0 and why we can't test that, this even impacts DBs.
#if !defined(MVIO) || defined(__AVR_ATmegax09__) || defined(Dx_48_PINS) || defined(Dx_64_PINS)
    pin_pd0     = 0x48,
#endif
    pin_pd1     = 0x49,
    pin_pd2     = 0x4A,
    pin_pd3     = 0x4B,
#endif
    pin_pd4     = 0x4C,
    pin_pd5     = 0x4F,
    pin_pd6     = 0x4E,
    pin_pd7     = 0x4F,
  };
};
#endif

// Features unique to event generator channel 4
#if defined(EVSYS_CHANNEL4)
namespace gen4 {
  enum generator_t : uint8_t {
    disable     = 0x00,
    off         = 0x00,
    rtc_div8192 = 0x08,
    rtc_div4096 = 0x09,
    rtc_div2048 = 0x0A,
    rtc_div1024 = 0x0B,
#if defined(PIN_PE0)
    pin_pe0     = 0x40,
    pin_pe1     = 0x41,
    pin_pe2     = 0x42,
    pin_pe3     = 0x43,
#endif
#if defined(PIN_PE4)
    pin_pe4     = 0x44,
    pin_pe5     = 0x45,
    pin_pe6     = 0x46,
    pin_pe7     = 0x47,
#endif
    pin_pf0     = 0x48,
    pin_pf1     = 0x49,
#if defined(PIN_PF2)
    pin_pf2     = 0x4A,
    pin_pf3     = 0x4B,
    pin_pf4     = 0x4C,
    pin_pf5     = 0x4D,
#endif
    pin_pf6     = 0x4E,
#if defined(PIN_PF7)
    pin_pf7     = 0x4F
#endif
  };
};
#endif

// Features unique to event generator channel 5
#if defined(EVSYS_CHANNEL5)
namespace gen5 {
  enum generator_t : uint8_t {
    disable     = 0x00,
    off         = 0x00,
    rtc_div512  = 0x08,
    rtc_div256  = 0x09,
    rtc_div128  = 0x0A,
    rtc_div64   = 0x0B,
#if defined(PIN_PE0)
    pin_pe0     = 0x40,
    pin_pe1     = 0x41,
    pin_pe2     = 0x42,
    pin_pe3     = 0x43,
#endif
#if defined(PIN_PE4)
    pin_pe4     = 0x44,
    pin_pe5     = 0x45,
    pin_pe6     = 0x46,
    pin_pe7     = 0x47,
#endif
    pin_pf0     = 0x48,
    pin_pf1     = 0x49,
#if defined(PIN_PF2)
    pin_pf2     = 0x4A,
    pin_pf3     = 0x4B,
    pin_pf4     = 0x4C,
    pin_pf5     = 0x4D,
#endif
    pin_pf6     = 0x4E,
#if defined(PIN_PF7)
    pin_pf7     = 0x4F
#endif
  };
};
#endif

// Features unique to event generator channel 6
#if defined(EVSYS_CHANNEL6)
namespace gen6 {
  enum generator_t : uint8_t {
    disable     = 0x00,
    off         = 0x00,
    rtc_div8192 = 0x08,
    rtc_div4096 = 0x09,
    rtc_div2048 = 0x0A,
    rtc_div1024 = 0x0B,
#if defined(PIN_PG0)
    pin_pg0     = 0x40,
    pin_pg1     = 0x41,
    pin_pg2     = 0x42,
    pin_pg3     = 0x43,
    pin_pg4     = 0x44,
    pin_pg5     = 0x45,
    pin_pg6     = 0x46,
    pin_pg7     = 0x47,
#endif
  };
};
#endif

// Features unique to event generator channel 7
#if defined(EVSYS_CHANNEL7)
namespace gen7 {
  enum generator_t : uint8_t {
    disable    = 0x00,
    off        = 0x00,
    rtc_div512 = 0x08,
    rtc_div256 = 0x09,
    rtc_div128 = 0x0A,
    rtc_div64  = 0x0B,
#if defined(PIN_PG0)
    pin_pg0     = 0x40,
    pin_pg1     = 0x41,
    pin_pg2     = 0x42,
    pin_pg3     = 0x43,
    pin_pg4     = 0x44,
    pin_pg5     = 0x45,
    pin_pg6     = 0x46,
    pin_pg7     = 0x47,
#endif
  };
};
#endif

// Features unique to event generator channel 8
#if defined(EVSYS_CHANNEL8)
namespace gen8 {
  enum generator_t : uint8_t {
    disable     = 0x00,
    off         = 0x00,
    rtc_div8192 = 0x08,
    rtc_div4096 = 0x09,
    rtc_div2048 = 0x0A,
    rtc_div1024 = 0x0B,
  };
};
#endif

// Features unique to event generator channel 9
#if defined(EVSYS_CHANNEL9)
namespace gen9 {
  enum generator_t : uint8_t {
    disable    = 0x00,
    off        = 0x00,
    rtc_div512 = 0x08,
    rtc_div256 = 0x09,
    rtc_div128 = 0x0A,
    rtc_div64  = 0x0B,
  };
};
#endif


// Generator users
namespace user {
  enum user_t : uint8_t {
#if defined(MEGACOREX)
    ccl0_event_a   = 0x00,
    ccl0_event_b   = 0x01,
    ccl1_event_a   = 0x02,
    ccl1_event_b   = 0x03,
    ccl2_event_a   = 0x04,
    ccl2_event_b   = 0x05,
    ccl3_event_a   = 0x06,
    ccl3_event_b   = 0x07,
    adc0_start     = 0x08,
    evouta_pin_pa2 = 0x09,
#if defined(PIN_PB2)
    evoutb_pin_pb2 = 0x0A,
#endif
    evoutc_pin_pc2 = 0x0B,
    evoutd_pin_pd2 = 0x0C,
#if defined(PIN_PE2)
    evoute_pin_pe2 = 0x0D,
#endif
    evoutf_pin_pf2 = 0x0E,
    usart0_irda    = 0x0F,
    usart1_irda    = 0x10,
    usart2_irda    = 0x11,
#if defined(USART3)
    usart3_irda    = 0x12,
#endif
    tca0           = 0x13,
    tca0_cnt_a     = 0x13,
    tcb0           = 0x14,
    tcb0_capt      = 0x14,
    tcb1           = 0x15,
    tcb1_capt      = 0x15,
    tcb2           = 0x16,
    tcb2_capt      = 0x16,
#if defined(TCB3)
    tcb3           = 0x17,
    tcb3_capt      = 0x17,
#endif
    // "Unofficial" user generators. Uses EVOUT, but swaps the output pin using PORTMUX
    evouta_pin_pa7 = 0x89,
#if defined(PIN_PC7)
    evoutc_pin_pc7 = 0x8B,
#endif
    evoutd_pin_pd7 = 0x8C,
#endif // defined(MEGACOREX)

#if defined(__AVR_DA__)
      ccl0_event_a   = 0x00,
      ccl0_event_b   = 0x01,
      ccl1_event_a   = 0x02,
      ccl1_event_b   = 0x03,
      ccl2_event_a   = 0x04,
      ccl2_event_b   = 0x05,
      ccl3_event_a   = 0x06,
      ccl3_event_b   = 0x07,
#if defined(LUT4)
      ccl4_event_a   = 0x08,
      ccl4_event_b   = 0x09,
      ccl5_event_a   = 0x0A,
      ccl5_event_b   = 0x0B,
#endif
      adc0_start     = 0x0C,
      ptc_start      = 0x0D,
      evouta_pin_pa2 = 0x0E,
#if defined(PIN_PB2)
      evoutb_pin_pb2 = 0x0F,
#endif
      evoutc_pin_pc2 = 0x10,
      evoutd_pin_pd2 = 0x11,
#if defined(PIN_PE2)
      evoute_pin_pe2 = 0x12,
#endif
#if defined(PIN_PF2)
      evoutf_pin_pf2 = 0x13,
#endif
#if defined(PIN_PG2)
      evoutg_pin_pg2 = 0x14,
#endif
      usart0_irda    = 0x15,
      usart1_irda    = 0x16,
      usart2_irda    = 0x17,
#if defined(USART5)
      usart3_irda    = 0x18,
#endif
#if defined(USART4)
      usart4_irda    = 0x19,
#endif
#if defined(USART5)
      usart5_irda    = 0x1A,
#endif
      tca0           = 0x1B,
      tca0_cnt_a     = 0x1B,
      tca0_cnt_b     = 0x1C,
      tca1           = 0x1D,
      tca1_cnt_a     = 0x1D,
      tca1_cnt_b     = 0x1E,
      tcb0_capt      = 0x1F,
      tcb0           = 0x1F,
      tcb0_cnt       = 0x20,
      tcb1_capt      = 0x21,
      tcb1           = 0x21,
      tcb1_cnt       = 0x22,
      tcb2_capt      = 0x23,
      tcb2           = 0x23,
      tcb2_cnt       = 0x24,
#if defined(TCB3)
      tcb3_capt      = 0x25,
      tcb3           = 0x25,
      tcb3_cnt       = 0x26,
#endif
#if defined(TCB4)
      tcb4_capt      = 0x27,
      tcb4           = 0x27,
      tcb4_cnt       = 0x28,
#endif
      tcd0_in_a      = 0x29,
      tcd0_in_b      = 0x2A,
      // "Unofficial" user generators. Uses EVOUT, but swaps the output pin using PORTMUX
      evouta_pin_pa7 = 0x8E,
#if defined(PIN_PB7)
      evoutb_pin_pb7 = 0x8F,
#endif
#if defined(PIN_PC7)
      evoutc_pin_pc7 = 0x90,
#endif
      evoutd_pin_pd7 = 0x91,
#if defined(PIN_PE7)
      evoute_pin_pe7 = 0x92,
#endif
      // evoutf_pin_pf7 = 0x93, never available on DA/DB
#if defined(PIN_PG7)
      evoutg_pin_pg7 = 0x94,
#endif
#endif // defined(__AVR_DA__)
#if defined(__AVR_DB__)
    ccl0_event_a   = 0x00,
    ccl0_event_b   = 0x01,
    ccl1_event_a   = 0x02,
    ccl1_event_b   = 0x03,
    ccl2_event_a   = 0x04,
    ccl2_event_b   = 0x05,
    ccl3_event_a   = 0x06,
    ccl3_event_b   = 0x07,
#if defined(LUT4)
    ccl4_event_a   = 0x08,
    ccl4_event_b   = 0x09,
    ccl5_event_a   = 0x0A,
    ccl5_event_b   = 0x0B,
#endif
    adc0_start     = 0x0C,
    evouta_pin_pa2 = 0x0D,
#if defined(PIN_PB2)
    evoutb_pin_pb2 = 0x0E,
#endif
    evoutc_pin_pc2 = 0x0F,
    evoutd_pin_pd2 = 0x10,
#if defined(PIN_PE2)
    evoute_pin_pe2 = 0x11,
#endif
#if defined(PIN_PF2)
    evoutf_pin_pf2 = 0x12,
#endif
#if defined(PIN_PG2)
    evoutg_pin_pg2 = 0x13,
#endif
    usart0_irda    = 0x14,
    usart1_irda    = 0x15,
    usart2_irda    = 0x16,
#if defined(USART3)
    usart3_irda    = 0x17,
#endif
#if defined(USART4)
    usart4_irda    = 0x18,
#endif
#if defined(USART5)
    usart5_irda    = 0x19,
#endif
    tca0          = 0x1A,
    tca0_cnt_a    = 0x1A,
    tca0_cnt_b     = 0x1B,
    tca1          = 0x1C,
    tca1_cnt_a    = 0x1C,
    tca1_cnt_b     = 0x1D,
    tcb0_capt      = 0x1E,
    tcb0_cnt       = 0x1F,
    tcb1_capt      = 0x20,
    tcb1_cnt       = 0x21,
    tcb2_capt      = 0x22,
    tcb2_cnt       = 0x23,
#if defined(TCB3)
    tcb3_capt      = 0x24,
    tcb3_cnt       = 0x25,
#endif
#if defined(TCB4)
    tcb4_capt      = 0x26,
    tcb4_cnt       = 0x27,
#endif
    tcd0_in_a      = 0x28,
    tcd0_in_b      = 0x29,
    opamp0_enable  = 0x2A,
    opamp0_disable = 0x2B,
    opamp0_dump    = 0x2C,
    opamp0_drive   = 0x2D,
    opamp1_enable  = 0x2E,
    opamp1_disable = 0x2F,
    opamp1_dump    = 0x30,
    opamp1_drive   = 0x31,
#if defined(OPAMP2)
    opamp2_enable  = 0x32,
    opamp2_disable = 0x33,
    opamp2_dump    = 0x34,
    opamp2_drive   = 0x35,
#endif
    // "Unofficial" user generators. Uses EVOUT, but swaps the output pin using PORTMUX
    evouta_pin_pa7 = 0x8D,
#if defined(PIN_PB7)
    evoutb_pin_pb7 = 0x8E,
#endif
#if defined(PIN_PC7)
    evoutc_pin_pc7 = 0x8F,
#endif
    evoutd_pin_pd7 = 0x90,
#if defined(PIN_PE7)
    evoute_pin_pe7 = 0x91,
#endif
#if defined(PIN_PG7)
    evoutg_pin_pg7 = 0x93,
#endif
#endif // defined(__AVR_DB__)

#if defined(__AVR_DD__)
    ccl0_event_a   = 0x00,
    ccl0_event_b   = 0x01,
    ccl1_event_a   = 0x02,
    ccl1_event_b   = 0x03,
    ccl2_event_a   = 0x04,
    ccl2_event_b   = 0x05,
    ccl3_event_a   = 0x06,
    ccl3_event_b   = 0x07,
    adc0_start     = 0x0C,
#if defined(PIN_PA2) // not on 14-pin ones.
    evouta_pin_pa2 = 0x0D,
#endif
    evoutc_pin_pc2 = 0x0F,
#if defined(PIN_PD2) // only on 28 or 32 pin ones.
    evoutd_pin_pd2 = 0x10,
#endif
#if defined(PIN_PF2) // only on 32-pin ones.
    evoutf_pin_pf2 = 0x12,
#endif
    usart0_irda    = 0x14,
    usart1_irda    = 0x15,
    tca0           = 0x1A,
    tca0_cnt_a     = 0x1A,
    tca0_cnt_b     = 0x1B,
    tcb0           = 0x1E,
    tcb0_capt      = 0x1E,
    tcb0_cnt       = 0x1F,
    tcb1           = 0x20,
    tcb1_capt      = 0x20,
    tcb1_cnt       = 0x21,
    tcb2           = 0x22,
    tcb2_capt      = 0x22,
    tcb2_cnt       = 0x23,
    tcd0_in_a      = 0x28,
    tcd0_in_b      = 0x29,
#if defined(PIN_PA7) // not on 14-pin ones.
    evouta_pin_pa7 = 0x8D,
#endif
#if defined(PIN_PC7)
    evoutc_pin_pc7 = 0x8F,
#endif
    evoutd_pin_pd7 = 0x90,
#endif // __AVR_DD__
  };
};
// END MegaCorex and DxCore definitions


// tinyAVR-2 definitions
#elif MEGATINYCORE_SERIES == 2
namespace gen {
  enum generator_t : uint8_t {
    disable         = 0x00,
    updi_synch      = 0x1,
    rtc_ovf         = 0x6,
    rtc_cmp         = 0x7,
    ccl0_out        = 0x10,
    ccl1_out        = 0x11,
    ccl2_out        = 0x12,
    ccl3_out        = 0x13,
    ac0_out         = 0x20,
    adc0_ready      = 0x24,
    adc0_sample     = 0x25,
    adc0_window     = 0x26,
    usart0_xck      = 0x60,
    usart1_xck      = 0x61,
    spi0_sck        = 0x68,
    tca0_ovf_lunf   = 0x80,
    tca0_hunf       = 0x81,
    tca0_cmp0       = 0x84,
    tca0_cmp1       = 0x85,
    tca0_cmp2       = 0x86,
    tcb0            = 0xA0,
    tcb0_capt       = 0xA0,
    tcb0_ovf        = 0xA1,
    tcb1            = 0xA2,
    tcb1_capt       = 0xA2,
    tcb1_ovf        = 0xA3,
  };
};

namespace gen0 {
  enum generator_t : uint8_t {
    disable     = 0x00,
    rtc_div8192 = 0x08,
    rtc_div4096 = 0x09,
    rtc_div2048 = 0x0A,
    rtc_div1024 = 0x0B,
    pin_pa0     = 0x40,
    pin_pa1     = 0x41,
    pin_pa2     = 0x42,
    pin_pa3     = 0x43,
    pin_pa4     = 0x44,
    pin_pa5     = 0x45,
    pin_pa6     = 0x46,
    pin_pa7     = 0x47,
    pin_pb0     = 0x40,
    pin_pb1     = 0x41,
    pin_pb2     = 0x42,
    pin_pb3     = 0x43,
#if defined(PIN_PB4)
    pin_pb4     = 0x44,
    pin_pb5     = 0x45,
#endif
#if defined(PIN_PB6)
    pin_pb6     = 0x46,
    pin_pb7     = 0x47,
#endif
  };
};

namespace gen1 {
  enum generator_t : uint8_t {
    disable     = 0x00,
    rtc_div512  = 0x08,
    rtc_div256  = 0x09,
    rtc_div128  = 0x0A,
    rtc_div64   = 0x0B,
    pin_pa0     = 0x40,
    pin_pa1     = 0x41,
    pin_pa2     = 0x42,
    pin_pa3     = 0x43,
    pin_pa4     = 0x44,
    pin_pa5     = 0x45,
    pin_pa6     = 0x46,
    pin_pa7     = 0x47,
    pin_pb0     = 0x40,
    pin_pb1     = 0x41,
    pin_pb2     = 0x42,
    pin_pb3     = 0x43,
#if defined(PIN_PB4)
    pin_pb4     = 0x44,
    pin_pb5     = 0x45,
#endif
#if defined(PIN_PB6)
    pin_pb6     = 0x46,
    pin_pb7     = 0x47,
#endif
  };
};

namespace gen2 {
  enum generator_t : uint8_t {
    disable     = 0x00,
    rtc_div8192 = 0x08,
    rtc_div4096 = 0x09,
    rtc_div2048 = 0x0A,
    rtc_div1024 = 0x0B,
#if defined(PIN_PC0)
    pin_pc0     = 0x40,
    pin_pc1     = 0x41,
    pin_pc2     = 0x42,
    pin_pc3     = 0x43,
#endif
#if defined(PIN_PC4)
    pin_pc4     = 0x44,
    pin_pc5     = 0x45,
#endif
    pin_pa0     = 0x40,
    pin_pa1     = 0x41,
    pin_pa2     = 0x42,
    pin_pa3     = 0x43,
    pin_pa4     = 0x44,
    pin_pa5     = 0x45,
    pin_pa6     = 0x46,
    pin_pa7     = 0x47,
  };
};

namespace gen3 {
  enum generator_t : uint8_t {
    disable     = 0x00,
    rtc_div512  = 0x08,
    rtc_div256  = 0x09,
    rtc_div128  = 0x0A,
    rtc_div64   = 0x0B,
#if defined(PIN_PC0)
    pin_pc0     = 0x40,
    pin_pc1     = 0x41,
    pin_pc2     = 0x42,
    pin_pc3     = 0x43,
#endif
#if defined(PIN_PC4)
    pin_pc4     = 0x44,
    pin_pc5     = 0x45,
#endif
    pin_pa0     = 0x40,
    pin_pa1     = 0x41,
    pin_pa2     = 0x42,
    pin_pa3     = 0x43,
    pin_pa4     = 0x44,
    pin_pa5     = 0x45,
    pin_pa6     = 0x46,
    pin_pa7     = 0x47,
  };
};

namespace gen4 {
  enum generator_t : uint8_t {
    disable     = 0x00,
    rtc_div8192 = 0x08,
    rtc_div4096 = 0x09,
    rtc_div2048 = 0x0A,
    rtc_div1024 = 0x0B,
    pin_pb0     = 0x40,
    pin_pb1     = 0x41,
    pin_pb2     = 0x42,
    pin_pb3     = 0x43,
#if defined(PIN_PB4)
    pin_pb4     = 0x44,
    pin_pb5     = 0x45,
#endif
#if defined(PIN_PB6)
    pin_pb6     = 0x46,
    pin_pb7     = 0x47,
#endif
#if defined(PIN_PC0)
    pin_pc0     = 0x40,
    pin_pc1     = 0x41,
    pin_pc2     = 0x42,
    pin_pc3     = 0x43,
#endif
#if defined(PIN_PC4)
    pin_pc4     = 0x44,
    pin_pc5     = 0x45,
#endif
  };
};

namespace gen5 {
  enum generator_t : uint8_t {
    disable     = 0x00,
    rtc_div512  = 0x08,
    rtc_div256  = 0x09,
    rtc_div128  = 0x0A,
    rtc_div64   = 0x0B,
    pin_pb0     = 0x40,
    pin_pb1     = 0x41,
    pin_pb2     = 0x42,
    pin_pb3     = 0x43,
#if defined(PIN_PB4)
    pin_pb4     = 0x44,
    pin_pb5     = 0x45,
#endif
#if defined(PIN_PB6)
    pin_pb6     = 0x46,
    pin_pb7     = 0x47,
#endif
#if defined(PIN_PC0)
    pin_pc0     = 0x40,
    pin_pc1     = 0x41,
    pin_pc2     = 0x42,
    pin_pc3     = 0x43,
#endif
#if defined(PIN_PC4)
    pin_pc4     = 0x44,
    pin_pc5     = 0x45,
#endif
  };
};


namespace user {
  enum user_t : uint8_t {
    ccl0_event_a            = 0x00,
    ccl0_event_b            = 0x01,
    ccl1_event_a            = 0x02,
    ccl1_event_b            = 0x03,
    ccl2_event_a            = 0x04,
    ccl2_event_b            = 0x05,
    ccl3_event_a            = 0x06,
    ccl3_event_b            = 0x07,
    adc0_start              = 0x08,
    evouta_pin_pa2          = 0x09,
    evouta_pin_pa7          = 0x89,
    evoutb_pin_pb2          = 0x0A,
#if defined(PIN_PB7)
    evoutb_pin_pb7          = 0x8A,
#endif
#if defined(PIN_PC2)
    evoutc_pin_pc2          = 0x0B,
#endif
    usart0_irda             = 0x0C,
    usart1_irda             = 0x0D,
    tca0                   = 0x0E,
    tca0_cnt_a             = 0x0E,
    tca0_cnt_b              = 0x0F,
    tcb0                    = 0x11,
    tcb0_capt               = 0x11,
    tcb0_cnt                = 0x12,
    tcb1                    = 0x13,
    tcb1_capt               = 0x13,
    tcb1_cnt                = 0x14,
  };
};


// tinyAVR-0/1 definitions
#else

namespace gen0 {
  enum generator_t : uint8_t {
#if defined(PIN_PC0)
    pin_pc0      = 0x07,
    pin_pc1      = 0x08,
    pin_pc2      = 0x09,
    pin_pc3      = 0x0A,
#endif
#if defined(PIN_PC4)
    pin_pc4      = 0x0B,
    pin_pc5      = 0x0C,
#endif
    pin_pa0      = 0x0D,
    pin_pa1      = 0x0E,
    pin_pa2      = 0x0F,
    pin_pa3      = 0x10,
    pin_pa4      = 0x11,
    pin_pa5      = 0x12,
    pin_pa6      = 0x13,
    pin_pa7      = 0x14,
#if (PROGMEM_SIZE > 8192 && MEGATINYCORE_SERIES == 1)
    tcb1         = 0x15,
    tcb1_capt    = 0x15,
#endif
  };
};
namespace gen2 {
  enum generator_t : uint8_t {
    pin_pa0      = 0x0A,
    pin_pa1      = 0x0B,
    pin_pa2      = 0x0C,
    pin_pa3      = 0x0D,
    pin_pa4      = 0x0E,
    pin_pa5      = 0x0F,
    pin_pa6      = 0x10,
    pin_pa7      = 0x11,
    updi         = 0x12,
#if (PROGMEM_SIZE > 8192 && MEGATINYCORE_SERIES == 1)
    ac1_out      = 0x13,
    ac2_out      = 0x14,
#endif
  };
};

Side note: This is illegal in terms of "Non-Arduino C++ rules": https://github.com/SpenceKonde/DxCore/blob/606d6c6210f74e13ba6ea296df448d0b485d8da1/megaavr/libraries/Event/src/Event.cpp#L239-L245
assign_generator_pin(uint8_t port, uint8_t port_pin) should be defined first, and then used in assign_generator_pin(uint8_t pin_number). This would probably not compile with PlatformIO.

@SpenceKonde
Copy link
Contributor

SpenceKonde commented Dec 11, 2021

Yeah I'm in the process of switching to testing PIN_Pxn defines in my copy.

I think a TINY_0_OR_1_SERIES will be more readable than the particular construction you cited

Thanks, good catch - hadn't realized it hadn't been declared yet.

also, am I missing something, or did we both not notice a chance to massively simplify set_user_pin....
if the port and port pin aren't NOT_A_PIN, and the port pin is either 2 or 7, we can just add the port number to the user number for evouta on pa2, or with 0x80 if port pin is 7, instead of a couple of dozen comparisons....

There is no part for which there is a port with a pin 2 or 7 on that doesn't have the option to do event out on it!

int8_t Event::set_user_pin(uint8_t pin_number) {
  uint8_t port = digitalPinToPort(pin_number);
  uint8_t port_pin = digitalPinToBitPosition(pin_number);

  int8_t event_user = -1;
  if (port != NOT_A_PIN && port_pin != NOT_A_PIN) {
    uint8_t evout_user = (uint8_t) user::evouta_pin_pa2;
    if (port_pin == 2) {
      event_user = (user::user_t)(evout_user + port); 
    } 
    #if !defined(TINY_0_OR_1_SERIES)
      else if (port_pin == 7) {
        event_user = (user::user_t)(evout_user + port); 
      }
    #endif
    else {
      return -1;
    }
    set_user((user::user_t)event_user);
  }
  return event_user;
}

Except that tiny 0/1-series don't have the pin7 option, of course.

@SpenceKonde
Copy link
Contributor

Can't combine the Dx's and mega0's as one would like to for the users - the DA's and DB's are off by 1 from eachother! (seriously microchip? )

@MCUdude
Copy link
Owner

MCUdude commented Dec 11, 2021

Yeah I'm in the process of switching to testing PIN_Pxn defines in my copy.

Great! Let me know if it works or not. The code is certainly easier to read, that's for sure. I've just pushed this very change to my repo. 03a8fac

I think a TINY_0_OR_1_SERIES will be more readable than the particular construction you cited

I would actually prefer TINY0_SERIES, TINY1_SERIES and TINY2_SERIES. Then we can let the macro logic do the work:

#if defined(TINY0_SERIES) || defined(TINY1_SERIES)
  // Ugly workaround code
#elif defined(TINY2_SERIES)
  // Elegant code
#endif

also, am I missing something, or did we both not notice a chance to massively simplify set_user_pin....
if the port and port pin aren't NOT_A_PIN, and the port pin is either 2 or 7, we can just add the port number to the user number for evouta on pa2, or with 0x80 if port pin is 7, instead of a couple of dozen comparisons....

Great catch! I didn't think of this. It's getting late here, so I'll have to continue tomorrow, but it would be great if you could give it a try!

Can't combine the Dx's and mega0's as one would like to for the users - the DA's and DB's are off by 1 from eachother! (seriously microchip? )

where exactly? EDIT: ah, you mean here. No code is broken, it's just a bit anoying.

#if defined(__AVR_DA__)
ccl0_event_a = 0x00,
ccl0_event_b = 0x01,
ccl1_event_a = 0x02,
ccl1_event_b = 0x03,
ccl2_event_a = 0x04,
ccl2_event_b = 0x05,
ccl3_event_a = 0x06,
ccl3_event_b = 0x07,
#if defined(LUT4)
ccl4_event_a = 0x08,
ccl4_event_b = 0x09,
ccl5_event_a = 0x0A,
ccl5_event_b = 0x0B,
#endif
adc0_start = 0x0C,
ptc_start = 0x0D,
evouta_pin_pa2 = 0x0E,
#if defined(PIN_PB2)
evoutb_pin_pb2 = 0x0F,
#endif
evoutc_pin_pc2 = 0x10,
evoutd_pin_pd2 = 0x11,
#if defined(PIN_PE2)
evoute_pin_pe2 = 0x12,
#endif
#if defined(PIN_PF2)
evoutf_pin_pf2 = 0x13,
#endif
#if defined(PIN_PG2)
evoutg_pin_pg2 = 0x14,
#endif
usart0_irda = 0x15,
usart1_irda = 0x16,
usart2_irda = 0x17,
#if defined(USART5)
usart3_irda = 0x18,
#endif
#if defined(USART4)
usart4_irda = 0x19,
#endif
#if defined(USART5)
usart5_irda = 0x1A,
#endif
tca0 = 0x1B,
tca0_cnt_a = 0x1B,
tca0_cnt_b = 0x1C,
tca1 = 0x1D,
tca1_cnt_a = 0x1D,
tca1_cnt_b = 0x1E,
tcb0_capt = 0x1F,
tcb0 = 0x1F,
tcb0_cnt = 0x20,
tcb1_capt = 0x21,
tcb1 = 0x21,
tcb1_cnt = 0x22,
tcb2_capt = 0x23,
tcb2 = 0x23,
tcb2_cnt = 0x24,
#if defined(TCB3)
tcb3_capt = 0x25,
tcb3 = 0x25,
tcb3_cnt = 0x26,
#endif
#if defined(TCB4)
tcb4_capt = 0x27,
tcb4 = 0x27,
tcb4_cnt = 0x28,
#endif
tcd0_in_a = 0x29,
tcd0_in_b = 0x2A,
// "Unofficial" user generators. Uses EVOUT, but swaps the output pin using PORTMUX
evouta_pin_pa7 = 0x8E,
#if defined(PIN_PB7)
evoutb_pin_pb7 = 0x8F,
#endif
#if defined(PIN_PC7)
evoutc_pin_pc7 = 0x90,
#endif
evoutd_pin_pd7 = 0x91,
#if defined(PIN_PE7)
evoute_pin_pe7 = 0x92,
#endif
// evoutf_pin_pf7 = 0x93, never available on DA/DB
#if defined(PIN_PG7)
evoutg_pin_pg7 = 0x94,
#endif
#endif // defined(__AVR_DA__)
#if defined(__AVR_DB__)
ccl0_event_a = 0x00,
ccl0_event_b = 0x01,
ccl1_event_a = 0x02,
ccl1_event_b = 0x03,
ccl2_event_a = 0x04,
ccl2_event_b = 0x05,
ccl3_event_a = 0x06,
ccl3_event_b = 0x07,
#if defined(LUT4)
ccl4_event_a = 0x08,
ccl4_event_b = 0x09,
ccl5_event_a = 0x0A,
ccl5_event_b = 0x0B,
#endif
adc0_start = 0x0C,
evouta_pin_pa2 = 0x0D,
#if defined(PIN_PB2)
evoutb_pin_pb2 = 0x0E,
#endif
evoutc_pin_pc2 = 0x0F,
evoutd_pin_pd2 = 0x10,
#if defined(PIN_PE2)
evoute_pin_pe2 = 0x11,
#endif
#if defined(PIN_PF2)
evoutf_pin_pf2 = 0x12,
#endif
#if defined(PIN_PG2)
evoutg_pin_pg2 = 0x13,
#endif
usart0_irda = 0x14,
usart1_irda = 0x15,
usart2_irda = 0x16,
#if defined(USART3)
usart3_irda = 0x17,
#endif
#if defined(USART4)
usart4_irda = 0x18,
#endif
#if defined(USART5)
usart5_irda = 0x19,
#endif
tca0 = 0x1A,
tca0_cnt_a = 0x1A,
tca0_cnt_b = 0x1B,
tca1 = 0x1C,
tca1_cnt_a = 0x1C,
tca1_cnt_b = 0x1D,
tcb0_capt = 0x1E,
tcb0_cnt = 0x1F,
tcb1_capt = 0x20,
tcb1_cnt = 0x21,
tcb2_capt = 0x22,
tcb2_cnt = 0x23,
#if defined(TCB3)
tcb3_capt = 0x24,
tcb3_cnt = 0x25,
#endif
#if defined(TCB4)
tcb4_capt = 0x26,
tcb4_cnt = 0x27,
#endif
tcd0_in_a = 0x28,
tcd0_in_b = 0x29,
opamp0_enable = 0x2A,
opamp0_disable = 0x2B,
opamp0_dump = 0x2C,
opamp0_drive = 0x2D,
opamp1_enable = 0x2E,
opamp1_disable = 0x2F,
opamp1_dump = 0x30,
opamp1_drive = 0x31,
#if defined(OPAMP2)
opamp2_enable = 0x32,
opamp2_disable = 0x33,
opamp2_dump = 0x34,
opamp2_drive = 0x35,
#endif
// "Unofficial" user generators. Uses EVOUT, but swaps the output pin using PORTMUX
evouta_pin_pa7 = 0x8D,
#if defined(PIN_PB7)
evoutb_pin_pb7 = 0x8E,
#endif
#if defined(PIN_PC7)
evoutc_pin_pc7 = 0x8F,
#endif
evoutd_pin_pd7 = 0x90,
#if defined(PIN_PE7)
evoute_pin_pe7 = 0x91,
#endif
#if defined(PIN_PG7)
evoutg_pin_pg7 = 0x93,
#endif
#endif // defined(__AVR_DB__)

@SpenceKonde
Copy link
Contributor

I've3 checked in the latest versions of my code. Testing was not particularly thorough.

1.4.0 and 2.5.0 are going out tonight come hell or high water, so that what has been added will be available to users - a TON of changes have gone in recently, and I want to get them into the hands of the people - and I'm moving and will not be able to devote time to this like I want to. So let's get something out, and if there are disasters, quick fixes can be pushed out, but no big new things for a while. (and when big new things are able ot happen again, ATTinyCore's final chapter is next in line).

@aikopras
Copy link
Author

I've tested both the MegaCoreX and DxCore variants and both work well (for my specific libraries). Thanks!!!

@MCUdude
Copy link
Owner

MCUdude commented Dec 14, 2021

Thanks for taking your time to check @aikopras! I only have a few minor things left before it's ready to be merged into the master branch.

@MCUdude
Copy link
Owner

MCUdude commented Dec 17, 2021

@SpenceKonde I think clear_user will work on any tinyAVR. It's missing PORTMUX handling for the tinyAVRs.

void Event::set_user(user::user_t event_user) {
  // Figure out what user register to write to based on the passed parameter
  uint8_t event_user_mask = event_user & 0x7F;
  #if defined(TINY_0_OR_1_SERIES)
    volatile uint8_t *user_register = &EVSYS_ASYNCUSER0 + (volatile uint8_t &)event_user_mask;
  #else
    volatile uint8_t *user_register = &EVSYS_USERCCLLUT0A + (volatile uint8_t &)event_user_mask;
  #endif
    // Connect user to the channel we're working with
    *user_register = channel_number + 1;
    // Set PORTMUX pin swap for EVOUT if selected as channel generator
  #if defined(TINY_0_OR_1_SERIES)
    if (event_user >= 0x08 && event_user <= 0x0A) {
      PORTMUX.CTRLA |= 1 << (event_user - 0x08);
    }
  #else
    if (event_user & 0x80) {
      #if defined(MEGACOREX)
        PORTMUX_EVSYSROUTEA |= (1 << ((event_user & 0x7F) - 0x09));
      #elif defined(__AVR_DA__)
        PORTMUX_EVSYSROUTEA |= (1 << ((event_user & 0x7F) - 0x0E));
      #elif defined(__AVR_DB__)
        PORTMUX_EVSYSROUTEA |= (1 << ((event_user & 0x7F) - 0x0D));
      #elif defined(__AVR_DD__)
        PORTMUX_EVSYSROUTEA |= (1 << ((event_user & 0x7F) - 0x0D));
      #elif MEGATINYCORE_SERIES == 2
        PORTMUX_EVSYSROUTEA |= (1 << ((event_user & 0x7F) - 0x0D));
      #endif
    }
  #endif
}


void Event::clear_user(user::user_t event_user) {
  // Figure out what user register to write to based on the passed parameter
  uint8_t event_user_mask = event_user & 0x7F;
  volatile uint8_t *user_register = &EVSYS_USERCCLLUT0A + (volatile uint8_t &)event_user_mask;

  // Disconnect from event generator
  *user_register = 0x00;

  // Clear PORTMUX pin swap for EVOUT if selected as channel user
  if (event_user & 0x80)  {
    #if defined(__AVR_ATmegax08__) || defined(__AVR_ATmegax09__)
      PORTMUX_EVSYSROUTEA &= ~(1 << ((event_user & 0x7F) - 0x09));
    #elif defined(__AVR_DA__)
      PORTMUX_EVSYSROUTEA &= ~(1 << ((event_user & 0x7F) - 0x0E));
    #elif defined(__AVR_DB__)
      PORTMUX_EVSYSROUTEA &= ~(1 << ((event_user & 0x7F) - 0x0D));
    #endif
  }
}

@MCUdude
Copy link
Owner

MCUdude commented Dec 19, 2021

The latest Event system library has now been merged into master.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants