SpenceKonde / megaTinyCore

Arduino core for the tinyAVR 0/1/2-series - Ones's digit 2,4,5,7 (pincount, 8,14,20,24), tens digit 0, 1, or 2 (featureset), preceded by flash in kb. Library maintainers: porting help available!
Other
551 stars 143 forks source link

Event library cannot be used to write portable code - it is always chained to a specific processor. #480

Closed SpenceKonde closed 2 years ago

SpenceKonde commented 3 years ago

We need the following in order to write generic code that makes use of the event system. The current Event library is essentially useless for this, as the user needs to know what the target processor is in order to get the constants they need to pass to the Event class's methods. The current implementation can only create ad-hoc code for specific parts on a case-by-case basis, which vastly undersells the power of the event system. I encountered this literally immediately when attempting to write anything beyond demonstration code.

Am I missing some way to do this sort of basic event systemy stuff with the current library implementation?

SpenceKonde commented 3 years ago

There is also no way to figure out what constant needs to be passed to get a specific pin as output either - if I am passing PIN_PA2 (a legal event output pin) around as a #define (you know, #define OUTPUT_PIN PIN_PA2 - the way everything else works here) - that ain't worth shit, because I need to pass the library user::evouta_pin_pa2. There is literally no way to get there from here with what is exposed to the user!

How do I get user::evouta_pin_pa2 from PIN_PA2?

digitalPinToPort() doesn't help me. digitalPinToBitPosition() doesn't help me.

The whole reason that people use Arduino is so that they don't have to deal with this shit, and this is the sort of thing that could and should be dealt with in a library.

MCUdude commented 3 years ago

Event::genToChannel(generator) - generator is either a pin number (minimal implementation) or a constant associated with each non-pin generator (preferred implementation). This static method will, starting from the highest numbered channel which can be connected to that generator, if it is connected to the specified generator. If it finds one, it will simply return a reference/pointer to that Event object. If none are found, but it does find a usable generator not pointed at anything, that should be connected to the generator, and a reference to irt returned (or should it be a pointer??? Does pointer work? If it does, that simplifies the error case) If there is no event channel that is either unused or pointing to that generator already, but can use that, return an indication of an error (null pointer? Reference to a dummy instance of Event? I don't know C++ man, I'm a C programmer!). Minimal implementation might instead just return the number of that channel. However in that case, we really need the next one

This should be possible to do. I don't have any hardware at the moment, but this compiles:

Event.h

//...
class Event
{
  public:
    Event(uint8_t channel_num, volatile uint8_t &channel_addr);
    static Event& get_generator_channel(uint8_t generator);
//...

Event.cpp

#include "Event.h"

// Pre-defined objects
#if defined(EVSYS_CHANNEL0)
  Event Event0(0, EVSYS_CHANNEL0);
  Event Event_empty(255, EVSYS_CHANNEL0);
#endif
#if defined(EVSYS_CHANNEL1)
  Event Event1(1, EVSYS_CHANNEL1);
#endif
#if defined(EVSYS_CHANNEL2)
  Event Event2(2, EVSYS_CHANNEL2);
#endif
#if defined(EVSYS_CHANNEL3)
  Event Event3(3, EVSYS_CHANNEL3);
#endif
#if defined(EVSYS_CHANNEL4)
  Event Event4(4, EVSYS_CHANNEL4);
#endif
#if defined(EVSYS_CHANNEL5)
  Event Event5(5, EVSYS_CHANNEL5);
#endif
#if defined(EVSYS_CHANNEL6)
  Event Event6(6, EVSYS_CHANNEL6);
#endif
#if defined(EVSYS_CHANNEL7)
  Event Event7(7, EVSYS_CHANNEL7);
#endif
#if defined(EVSYS_CHANNEL8)
  Event Event8(8, EVSYS_CHANNEL8);
#endif
#if defined(EVSYS_CHANNEL9)
  Event Event9(9, EVSYS_CHANNEL9);
#endif

/**
 * @brief Construct a new Event object
 *
 * @param channel_num Event channel number
 * @param channel_addr Register address to channel generator
 */
Event::Event(uint8_t channel_num, volatile uint8_t &channel_addr)
  : channel_number(channel_num), channel_address(channel_addr)
{
}

Event& Event::get_generator_channel(uint8_t generator)
{
  #if defined(EVSYS_CHANNEL0)
    if(Event0.generator_type == generator)
      return Event0;
  #endif
  #if defined(EVSYS_CHANNEL1)
    if(Event1.generator_type == generator)
      return Event1;
  #endif
  #if defined(EVSYS_CHANNEL2)
    if(Event2.generator_type == generator)
      return Event2;
  #endif
  #if defined(EVSYS_CHANNEL3)
    if(Event3.generator_type == generator)
      return Event3;
  #endif
  #if defined(EVSYS_CHANNEL4)
    if(Event4.generator_type == generator)
      return Event4;
  #endif
  #if defined(EVSYS_CHANNEL5)
    if(Event5.generator_type == generator)
      return Event5;
  #endif
  #if defined(EVSYS_CHANNEL6)
    if(Event6.generator_type == generator)
      return Event6;
  #endif
  #if defined(EVSYS_CHANNEL7)
    if(Event7.generator_type == generator)
      return Event7;
  #endif
  #if defined(EVSYS_CHANNEL8)
    if(Event8.generator_type == generator)
      return Event8;
  #endif
  #if defined(EVSYS_CHANNEL9)
    if(Event9.generator_type == generator)
      return Event9;
  #endif

  #if defined(EVSYS_CHANNEL0)
    else
      return Event_empty;
  #endif
}
//...

This would let you call Event::get_generator_channel(gen::ccl0_out); and it will return a reference to the object assigned to the ccl0_out generator. This again means that you should be able to get the actual event channel number like so:

// Get channel number gen::ccl0_out is used with
Event::get_generator_channel(gen::ccl0_out).get_channel_number();

// Alternative
Event& myEvent = Event::get_generator_channel(gen::ccl0_out);
myEvent.get_channel_number();

Event::numberToChannel(channel number) - return a reference to the EventN. Absolutely not optional in above case. Otherwise possibly optional.

Wouldn't get_channel_number() do exactly this? I've read this sentence many times, and it's a bit ambiguous... Care to explain what you really mean so I can wrap my head around it?

Event::getGenerator*(something to identify a specific generator - not sure what the right way is yet, channel number) - return the genN::generatorname constant associated with the specified generator.

I'm not sure what you mean here. EventN.get_generator() already exists if you need to know what generator is used for a particular channel.

How do I get user::evouta_pin_pa2 from PIN_PA2?

Good luck figuring that out! I've tried and figured it's never going to be elegant since you can't use an arbitrary pin for this. It's also channel-dependent which makes it even less intuitive. I know it's a bit painful, but that's why we have documentation.

SpenceKonde commented 3 years ago

Say I'm implementing a function PulseInViaCapture - use input capture to do this instead of cycle counting. Since it's an example, then we just wait in a while. loop checking on a status flag, but anyway

The user tells my library what timer to us, and what pin to use.

Now I try to implement.... if he wanted PB1., it's easy to use the macros to figure out port and bit that corresponds toBut to use this library that isn't any good, We can't set the event channel with it! We need gen0::pin_pb1 .Code can find that it's in position 1 and port B.

Basically, my problems is, supplied with pin and peripheral numbert provided by the end user's code, how can I set up input capture for them on that pin?

That's almost as simple as it can possibly get for events. And our library is powerless there. "Use TCB1" (okay I know that means i need that as my generator) to time PB2 (okay, that's the other part)

To implment the event connecrtion, I'd need to call: Event1.setGenerator(gen1::pin_pb1), Event1.setIUser(user::tcb1_capt); (the rest of it is beyond the scope of this issue, and easy). How do I do that, if the user passed me a pin number amd timer number???

There does not appear to be any remotely graceful way....

by numberToChannel, I mean, my code has an integer that happens to have the value 2. Because that was passed from the user code when I asked it which event generator I should look at. But the number 2 doesn't do me any good when I need Event2 object. How do you cross between those universes?

I agree that the whole concept is really messy, bit this [I count event and logic too) is also the most important peripeheral in the AVR architecture today and the competition doesn't even come close; and it will continue to be for the next decade or longer.,

I think we may have different goals in mind too - My main concern, by far,is that people can build on these libraries to make a second lever library that let tyou take advantage of that connection to do something. channels to do stuff. Timer compare, synchronizing ADC with external pulses, (people seem to care about that shrug), and , since they all use Event.,h they would not be mutually incompatible by stepping on eachother's toes (registers).

Receive on any RC-switch or RF OOK modules written for classic AVR will be broken here. This.... is really kind of a problem. The changes would be mad easy to make one one person did it and you'd see n what he changed - it'd be practically cookbook after that, except for that translating between pin number and generator. which you need to connect the pin change event to the timer.

But there's no way to

Event doesn't provide a way to use these events without case-by-case part specific countermeasures, even though the peripherals are virtually identical and the functionality is directly comparable.

Similarly, I'm thinking about how these references will be passed around so that those libraries wouldn't run into ]roadblocks we unwittingly set for them, (Okay I've got one library already using a this generator channel, I need another for something else, simple event call to get the available one to pass to the Event method?

You seem to have come at it from a totally different angle, assuming a human user is directly using the event library.

I am betting that the vast majority weill only use a library that does RC-switch, or input capture, remap inconveniently located PWMs on tinyAVRs... or DX/Mega for that matter..... or a bunch of other cool stuff these parts could do burt currently don't have the library infrastructure for. They wouldn't touch something as low level as Event or even Logic. (I have to say, I consider logic and event to be so closely linked that they're impossible to imagine separately.

In my mind, EVSYS is on-chip peripherals what SPI or TWI is to external ones. Imagine if those stock libraries didn't have enough functionality to do what someone needed? The never I2C device would have it's own slightly different internal implementation, and using two at once, they;'d fight each other and it would be awful (reference: classic TinyAVR parts with a USI - and s'it's what 6 years after I introduced Universal Wire and Universal SPI libraries')

Do you see where I'm coming from? I'd hoped this would be a component of (to start with) and input capture library - but right out of the gate, this doesn't get us any closer. It's just a moderate help for people manually working with the event system. There is so much potential here!

MCUdude commented 3 years ago

You should bear in mind that the Event library I've created is a pretty thin layer over the registers, and doesn't provide that much abstraction other than making the syntax readable for a human. It was also written to be used "manually", but I'd love to make it work within libraries too. But that requires some work, absolutely!

The main issue is that it's difficult to create a lightweight library that would "smooth" out all the quirks of the event system and its registers. I'm sure it's possible, but it requires some thought and understanding of many of the use cases of the library. But we can do that.

We could make it so that static functions (Event::) is what's used to handle "high level" settings such as Arduino pin numbers, figuring out timers etc, and all the event objects (Event, Event1...) is what actually interfaces directly with the Event hardware.

We probably need a higher level Event wrapper (not just an interface like we have today) that can deal with high-level stuff like Arduino pins, timer objects, and maybe even CCL/Logic objects.

I can do this, but I need to know exactly what goes into the functions and what should be returned (numbers, object references etc.). Can you write a list of the function we would need with short and precise descriptions of what goes onto and out of the functions? This would make my job easier, as I don't need to try to figure out myself what I think you mean we need. Something like this:

// Takes the passed generator type (gen::ccl0_out for instance) and returns a reference to the object it's currently being used with.
// Input: generator
// Returns: Reference to an object
Event::get_generator_channel(uint8_t generator);

// Takes the channel number as input, and returns the corresponding channel number object
// Input: channel number
// Returns: Reference to an object
Event::get_channel(uint8_t channel_number);
SpenceKonde commented 3 years ago

Thank you <3 that would be amazing, I will workout some specs for this - with care and as much planning as I can muster, because I want to get this right :-P

MCUdude commented 3 years ago

Any updates on the specs?

SpenceKonde commented 3 years ago

No. this is a siuper high priority, but the other priorities are even more critical. sorry. 2.4.0 doesn't even work and there are 2 critical; bugs in DX-core and there is no viable core for many of the c;lassic tinies due to the 1.5.0 - 2.0.0 divide on attiny core, sorry.

MCUdude commented 3 years ago

Looks like you have a lot on your plate! Let me know when you're ready 👍

MCUdude commented 3 years ago

I've implemented most of the features you've requested. This is currently what's new:

I have yet to figure out a clever way to handle event users differently such as Arduino pins and timer objects.

SpenceKonde commented 2 years ago

next release is now 2.5.0

SpenceKonde commented 2 years ago

Done and in.