MCUdude / MegaCoreX

An Arduino hardware package for ATmega4809, ATmega4808, ATmega3209, ATmega3208, ATmega1609, ATmega1608, ATmega809 and ATmega808
GNU Lesser General Public License v2.1
242 stars 47 forks source link

Event::clear_user() not declared static #106

Closed SpenceKonde closed 3 years ago

SpenceKonde commented 3 years ago

Looks like it's supposed to be declared static (per readme)... but it's not, what readme suggests doesn't work:

#include <Event.h>

void setup() {
  Event0.soft_event();
  //Event::clear_user(user::tcb0_capt);
}

void loop() { /* nop */}

C:\Users\Spence\Documents\Arduino\sketch_jan20a\sketch_jan20a.ino: In function 'void setup()': sketch_jan20a:6:36: error: cannot call member function 'void Event::clear_user(user::user_t)' without object Event::clear_user(user::tcb0_capt); ^

Also not all Dx-series devices have more than 8 channels, so I changed the soft_event() method to

void Event::soft_event()
{
  // Write to the bit that represent the channel in the strobe register
  #if defined(EVSYS_STROBE)
    // megaAVR 0-series
    EVSYS.STROBE = (1 << channel_number);
  #elif defined (EVSYS_ASYNCCH0)
    // tinyAVR 0/1-series
    // TODO: Add support the the dreadful implementation of events here
  #else
    // This is a civilized part which uses the 2020 version of EVSYS
    // we expect there to be an EVSYS.SWEVENTA channel plus an
    // EVSYS.SWEVENTB it it has more than 8 event channels.
    #if defined(EVSYS_SWEVENTB)
      if(channel_number < 8)
        EVSYS.SWEVENTA = (1 << channel_number);
      else
        EVSYS.SWEVENTB = (1 << (channel_number - 8));
    #else
    EVSYS.SWEVENTA = (1 << channel_number);
    #endif
  #endif
}
SpenceKonde commented 3 years ago

Also the README lists some pinswaps that don't exist on the megaAVR 0-series (or at all): ports B and E: The 48-pin parts don't have a PB7 or a PE7. Hence evoute_pin_pe7 and evoute_pin_pb7 shouldn't exist for the 0-series.

PF7 is the UPDI pin. It cannot be reconfigured to act as a GPIO pin on the megaAVR 0-series, AVR DA-series, or AVR DB-series (though in the AVR DD-series, PF7 can be set as GPIO; it gets un-GPIOified with an HV pulse... on the RESET pin (as opposed to the tinyAVR 0/1-series where that HV pulse goes to RESET/UPDI). Hence, it also shouldn't be there for MegaCoreX.

MCUdude commented 3 years ago

Looks like it's supposed to be declared static (per readme)... but it's not, what readme suggests doesn't work:

Thanks, it is supposed to be static. Fixed!

Also the README lists some pinswaps that don't exist on the megaAVR 0-series (or at all): ports B and E: The 48-pin parts don't have a PB7 or a PE7. Hence evoute_pin_pe7 and evoute_pin_pb7 shouldn't exist for the 0-series.

There is no reference to evoute_pin_pe7 or evoutb_pin_pb7 anywhere for the megaAVR-0. If I'm wrong, can you point me in the right direction? What is wrong is that you can use these pins as event generators, even though they don't exist. Do you have ideas on how this can be solved, while still maintaining readable code without hundreds of #if defined(...)? But I'm removing pin_pf7 regardless.

MCUdude commented 3 years ago

Maybe we could use:

// Helper macros to prevent us from having to check for every possible target

// megaAVR-0
#if defined(__AVR_ATmega4809__) || defined(__AVR_ATmega4808__)   \
||  defined(__AVR_ATmega3209__) || defined(__AVR_ATmega3208__)   \
||  defined(__AVR_ATmega1609__) || defined(__AVR_ATmega1608__)   \
||  defined(__AVR_ATmega809__)  || defined(__AVR_ATmega808__)
  #define MEGAAVR_0
#endif
#if defined(__AVR_ATmega4809__) || defined(__AVR_ATmega3209__)   \
||  defined(__AVR_ATmega1609__) || defined(__AVR_ATmega809__)
  #define MEGAAVR_0_48_PIN
#elif defined(__AVR_ATmega4808__) || defined(__AVR_ATmega3208__) \
||  defined(__AVR_ATmega1608__) || defined(__AVR_ATmega808__)
  #define MEGAAVR_0_32_28_PIN
#endif

// AVR-DA
#if defined(__AVR_AVR128DA64__) || defined(__AVR_AVR128DA48__)   \
|| defined(__AVR_AVR128DA32__)    || defined(__AVR_AVR128DA28__) \
|| defined(__AVR_AVR64DA64__)     || defined(__AVR_AVR64DA48__)  \
|| defined(__AVR_AVR64DA32__)     || defined(__AVR_AVR64DA28__)  \
|| defined(__AVR_AVR32DA48__)     || defined(__AVR_AVR32DA32__)  \
|| defined(__AVR_AVR32DA28__)
  #define AVR_DA
#endif
#if defined(__AVR_AVR128DA64__) || defined(__AVR_AVR64DA64__)
  #define AVR_DA_64_PIN
#elif defined(__AVR_AVR128DA48__) || defined(__AVR_AVR64DA48__)  \
|| defined(__AVR_AVR32DA48__)
  #define AVR_DA_48_PIN
#elif defined(__AVR_AVR128DA32__) || defined(__AVR_AVR64DA32__)  \
|| defined(__AVR_AVR32DA32__)
  #define AVR_DA_32_PIN
#elif defined(__AVR_AVR128DA28__) || defined(__AVR_AVR64DA28__)  \
|| defined(__AVR_AVR32DA28__)
  #define AVR_DA_28_PIN
#endif

// AVR-DB
#if defined(__AVR_AVR128DB64__) || defined(__AVR_AVR128DB48__)   \
|| defined(__AVR_AVR128DB32__)    || defined(__AVR_AVR128DB28__) \
|| defined(__AVR_AVR64DB64__)     || defined(__AVR_AVR64DB48__)  \
|| defined(__AVR_AVR64DB32__)     || defined(__AVR_AVR64DB28__)  \
|| defined(__AVR_AVR32DB48__)     || defined(__AVR_AVR32DB32__)  \
|| defined(__AVR_AVR32DB28__)
  #define AVR_DB
#endif
#if defined(__AVR_AVR128DB64__) || defined(__AVR_AVR64DB64__)
  #define AVR_DB_64_PIN
#elif defined(__AVR_AVR128DB48__) || defined(__AVR_AVR64DB48__)  \
|| defined(__AVR_AVR32DB48__)
  #define AVR_DB_48_PIN
#elif defined(__AVR_AVR128DB32__) || defined(__AVR_AVR64DB32__)  \
|| defined(__AVR_AVR32DB32__)
  #define AVR_DB_32_PIN
#elif defined(__AVR_AVR128DB28__) || defined(__AVR_AVR64DB28__)  \
|| defined(__AVR_AVR32DB28__)
  #define AVR_DB_28_PIN
#endif

to mask out different features only present on a specific chip? I'll suggest we first start with the enums in Event.h to remove pins and other features not present. Agree?

SpenceKonde commented 3 years ago

There's gotta be something we can use from the io header file? (like how in soft_event() I check the existence of registers) - cleaner code, and better odds that future chips will sort part of their functionality out on their own.

SpenceKonde commented 3 years ago

Eg, for the event system users... there are EVSYS_USER(name of event user) #defines!

That said - I wasn't demanding blocking out the ones that don't exist on a specific chip (though long term we probably should), just that like, the 0-series list and readme probably shouldn't have users listed that no part in the product line has available.

MCUdude commented 3 years ago

There's gotta be something we can use from the io header file? (like how in soft_event() I check the existence of registers) - cleaner code, and better odds that future chips will sort part of their functionality out on their own.

That's a better idea. I'll have to get my daughter from kindergarten now, and I won't be available until another two hours. However, if you're available a little later, I'd like to continue working on this with you!

SpenceKonde commented 3 years ago

Aaha! They're not in the readme - or they weren't at least until I was regenerating pieces of the table for DxCore with incautious regexes...

Oh..... I see... I got tripped up by not noticing what was in which #define

SpenceKonde commented 3 years ago

You know what's really annoying? How you can;t check for enum members with #ifdef I don't actually see any #defines that will tell you which pins actually exist on a part!

I actually also just realized... that it's going to be a part of DxCore... Arduino.h from DxCore provides:

__AVR_DA__
__AVR_DB__

As well as defines of the form:

DA_64_PINS
DB_64_PINS
Dx_64_PINS

Does your MegaCoreX not do something similar?

SpenceKonde commented 3 years ago

Does EVSYS override PORTx.DIR - or is it the responsibility of the user to ensure (one of my favorite phrases) that the pin is set as an output? If it is, it would be sporting to let them know in the readme. But what brought that to my mind is that... why not just #include pin_arduino and check for PIN_Pxn which is defined for all pins that exist... (at least for DxCore - and yours if you have these defines, which IMO you ought to).

Oh, also, on the AVR DA parts, there's apparently one extra generator which they removed from the headers... (same time they removed mention of a 4x multiplier from the TCD PLL, and an extra-low-power setting from the ACs). I think they'd originally planned on the chip running at up to 32 MHz with the PLL running at twice that.... then discovered they couldn't do that across the whole temperature range.... (the internal oscillator can be set 1, 2, 3, 4, 8, 12, 16, 20 or 24 MHz... or if you continue the pattern into the "reserved" values 28, and 32. Beyond that it repeats the last 4 for the remaining value possible in the 4-bit bitfield). Right after the last documented user, there's a "USEROSCTEST". I haven't had a chance to try setting it, making a timer output a 1kHz squarewave and firing it to see what it does :-P (somewhat related - the early io headers for the tiny 2-series accidently left in headers for a SRAM selftest - very curious as to whether test peripherals like that are still present on production silicon - and if they are, I dearly want a copy of the headers that their engineers get! There's gotta be a field of strange and mostly useless testing functionality strewn about in the address space.... somewhat related, I fantasize about systematically trying the "reserved" values to see if any of them do anything weird... I mean, we now know what the reserved value in the PLL multiplication factor field does!)

MCUdude commented 3 years ago

Arduino.h from DxCore provides: AVR_DA AVR_DB As well as defines of the form: DA_64_PINS DB_64_PINS Dx_64_PINS Does your MegaCoreX not do something similar?

Yes, MegaCoreX does. I've just pushed a fix. BTW can you fill in what peripherals are available for the DA's and DB's under the user enum here: https://github.com/MCUdude/MegaCoreX/blob/master/megaavr/libraries/Event/src/Event.h#L372-L529 ? It takes a ton of time for me to do it since I don't know the DA/DB peripherals very well. You don't need to provide a PR, I just need somewhere to copy your "fix" from.

Does EVSYS override PORTx.DIR - or is it the responsibility of the user to ensure (one of my favorite phrases) that the pin is set as an output? If it is, it would be sporting to let them know in the readme. But what brought that to my mind is that... why not just #include pin_arduino and check for PIN_Pxn which is defined for all pins that exist... (at least for DxCore - and yours if you have these defines, which IMO you ought to).

The pin gets automatically set to an output. At least, the output pin can both sink and source current, so I'm pretty sure the EVSYS takes care of setting the output register. Instead of checking every pin, you can see my latest commit on how I solved it. I just used your pre-defined macros to determine what target we're working with.

SpenceKonde commented 3 years ago

Yup working on it

Also... GAAAAAA THEY KNEW! THEY KNEW WHEN NUMBERING GENERATORS ON THE 4809

    EVSYS_GENERATOR_TCB0_CAPT_gc = (0xA0<<0),  /* Timer/Counter B0 capture */
    EVSYS_GENERATOR_TCB1_CAPT_gc = (0xA2<<0),  /* Timer/Counter B1 capture */
    EVSYS_GENERATOR_TCB2_CAPT_gc = (0xA4<<0),  /* Timer/Counter B2 capture */
    EVSYS_GENERATOR_TCB3_CAPT_gc = (0xA6<<0),  /* Timer/Counter B3 capture */

THAT THEY WERE GOING TO MAKE AN OVF ONE TOO!

MCUdude commented 3 years ago

Yup working on it

Awesome! I'll just copy yours when it's ready.

THAT THEY WERE GOING TO MAKE AN OVF ONE TOO!

I wonder if OVF is supported/working on the megaAVR-0, but just left it out because of errata or similar? I wouldn't be surprised.