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

New library (or libraries): TimerB #481

Closed SpenceKonde closed 1 year ago

SpenceKonde commented 3 years ago

We really ought to have a minimal wrapper class for at least the two most common non-PWM uses of a type B timer, and ideally the third as well. These are:

  1. One-shot mode, to generate a pulse of specified length
  2. Input capture; one can get way fancy here, but we really oughtn't. I would be overjoyed with an implementation that was just good enough to to OOK RF and IR receiving without the user having to manipulate raw registers and In short, users ought to be able to do something like:
    void my_function(uint16_t pulselength) {
    //function gets called when the TCB capture interrupt fires 
    }
    // Elsewhere:
    TimerB.capturePulse(uint8_t tcbnumber. uint8_t pin, uint8_t polarity [HIGH or LOW],void *my_function, [one or more additional parameters])

    The additional parameters should include an option to repeat or not-repeat, at minimum. Preferably, this should allow more complexity - see the Modes section for TimerB

  3. Periodic interrupt mode - User provides a function to be called from the ISR.
  4. Event Counter (2-series only)

Notes: All of these require that the ISR, and some timer-specific call, be located in a separate .c file, and hence in a separate compilation unit, so that the ISRs do not generate duplicate vector errors when not used.

These are also currently blocked by #480 - I want to use the Event library for this, and if we don't have an event library capable of this, then either the current one must be extended or replaced to rectify this.

grandaspanna commented 3 years ago

Given that the possible results of the ISR are going to be a pretty small set, would a default ISR in the library with a defined structure (result_updated/value(s)/overflow) make sense?

SpenceKonde commented 3 years ago

What do you mean by this? There are often things that need to be done in the ISR with the duration captured by the input capture module; Consider Input Capture on Event mode where the user needs to know the duration of both the highs and the lows (this is often critical for OOK RF receiving for example). The only way I can see to do this is to use input capture pom event ,and swap Edge during every ISR - plus also do some logic on that captured duration too - if state = idle, check if it'sthe right length for a start pulse, if it's in rx state, check for whether it's a 0, 1, portion of rthe signal that has nothing to say either way (in the protocol I made for AzzyRF, only high has meaning, but the lows between them had to be within a range to be valid., or an invalid length in which case we we reset the state back to idle. In that sort of use case, you cannot rely on loop running between each bit to process the latest capture.

SpenceKonde commented 2 years ago

Definitely requires some tough design decisions, The overhead of simply calling a function from an ISR is sort of shocking.

SpenceKonde commented 2 years ago

The capabilities of the type B timers are being ignored, and that is unjust, The type B timers deserve better!

I am inclined to not impement this as classes due to the overhead and the fact that the compiler can't seem to optimize them worth half a crap.

There are a considerable number of ways that this could be implemented ranging from efficient to abominable, and readily usable to incomprehensible.

TragicQuadrant
ObviousInRetrospect commented 2 years ago

so given the overhead of a callback function invocation from the ISR (are inline weak functions a thing?) what about some wrappers around the register manipulation + some examples for how to do these things while letting the user own the ISR might end up working better than a fully featured library here

SpenceKonde commented 2 years ago

Yeah, I personally favor a library that doesn't define the ISR and instead shows how youd use it in the examples - but frankly for the TCBs there are a lot of cases where you don't even need an interrupt that does more than clear the intflag set another flag and read the capture register and maybe not even that.

I mean a TCBOneShot library? All the interrupt would need to do is clear the flag so it would't get caught in an interrupt death spiral. And the rest is writing 5 bytes or so to the TCB control registers. input capture isn't much worse. The hard part is the event library, not anything in TCB - we already have wrappers around that.

but right now, I'm, what's the phrase "Up to my face in alligators", (you know, how the head of none of the three cores will compile bare minimum for starters), this isn't happening any time soon unless I end up inadvertantly making an example for something else that I could chop out and include,

Hence why the milestone is "some future version"

SpenceKonde commented 2 years ago

Oh - and as far as library priorities go, this is miles below sleeplib the planned sleep and power consumption library.

ObviousInRetrospect commented 2 years ago

anything stuck where another set of eyes would be useful?

SpenceKonde commented 2 years ago

What angle are you coming at this from? I mean, do you know how to use the TCB, and want others to use it too, or are you having TCB problems and want some libraries to help make this work?

Also, are you one of the PIO folks? I don't even know what PIO needs, but the other threads made it sound like I should be providing more resources for PIO; if you are familiar with it, I'd like to get an explanation, so we can figure out how to achieve it in light of the new boards.txt builders (I;llbe checking in a new version soon - I have code that isn't checked in for mTC) but I have this problem with the work I did for a client last week and eaglecad has me in it's talons and is ascending a thermal towards it's nest to feed me to it's eglets (I think that's what baby eagles are called?) anyway I'm on the fifth try after corrupting the files four times and ending up with unusable results. Just trying to rename nets so they match another design. (issue was that he told me two wrong connections but had I named the nets the same way, the problem would have been caught, and yeah, it's impossible to compare the two designs the way they're named now, he's right.

(based on the issues and comments I get, more people are using the my mTC and DxC with PIO than with Arduino, or maybe they just have more problems because I don't make life easy for them, because I know about as much asbout PIO as my cat knows about AVR assembly. (and to be clear, she doesn't know AVR assembly, and doesn't even claim to by walking on the keyboard like some cats to)

ObviousInRetrospect commented 2 years ago

I was coming at this particular one from the angle of trolling through the issues to see if there was anything I might be able to help with and having a potential approach occur to me and so I wrote a comment suggesting it. In general I've found TCB to be powerful but every time I have tried to use it I ended up spending hours staring at the data sheet + a bunch of trial and error to get it to do what I want so I guess I'm on the latter camp. I also tend to use non-TCB approaches where TCB would be a better solution because I know the worse approaches will work and be working much faster. For example, my clock calibration should have been written using TCB.

I am mostly not one of the PIO folk. I'm kind of surprised by the contrast of your approach to Arduino 2.0 (my perception is you think it is unsupported beta crap that you don't want to think about) vs your desire to support other alternate environments like PIO. Pre Arduino 2.0 I sometimes used PIO because it is a better IDE than 1.x. But at least when I used it I found the ability to fire up an example hard to use, the configuration hard to use, and found myself running into many problems where various defines weren't set right. At this point the one killer feature PIO has for me is the ability to set global defines. Arduino 2.0 now has library version management and some amount of settings management that is nearly as good. So I haven't used PIO in quite a while and was never really comfortable in it.

From my experience with PIO I suspect the disproportionate reports are more a sign of it causing problems than it being most of the usage. I would suggest that you discourage its use by non-experts unless and until you get a community member who understands it well and is willing to help. The pull request documenting how to set it up seemed counterproductive as it mostly helped people get started down a badly documented path but wasn't deep enough to help people avoid/solve the issues.

SpenceKonde commented 1 year ago

The reason I'm not working on Arduino 2.0.x is that 1) most of the issues being reported aren't bugs I can fix, they're regressions in the IDE that need to be corrected by the Arduino team. Clearly from the volume and nature of reports of problems,. 2.0.x is not yet ready for prime time. and 2) It's not a low priority per se, but there are months of higher priority work ahead of it in the queue - that matters more to more users, and I already work on the cores almost full time. So I've gotta set priorities, and supporting a known buggy version of the IDE comes below making everyhing work on the last release that was actually usable. and I'm hopping by then it will actually be mature and usable and half the problems will have gone away as they unbreak stuff. I don't think there's any way around the fact that the initial 2.0.x releases were shitshows and still are.

Re: PIO, we have a HUGE number of users who use it, and they start gathering outside my apartment with pitchforks if I don't do what they tell me. And usually they will tell me exactly what I need to do, while the Arduino bugs also require figuring out what's wrong, and doing a full investigation. Lets not forget that only maybe 20% of Arduino IDE releases historically have been remotely fit for prupose :-/

SpenceKonde commented 1 year ago

Regarding the original matter of a TCB library or libraries: I think it would be very nice to have a way to set up input capture, since now it's a multiphase process, where the EVSYS and TCB both need matching configurations. Either by using event.h's assign funcions, or implementing channel selection yourself. I will note that in typical input caputure modes, speed is enough of a concern that an attachInterrupt like mechanism makes the library DOA. I don't see a way around the user having to do the raw ISR themselves to get acceptable operformance in most cases.

Right now, baically nobody uses input capture. Why? Because it's too complicated and there are neither examples showing how to do it nor libraries for it....

hmeijdam commented 1 year ago

If my name was "nobody" your statement is accurate "Nobody uses it". Or do you refer to just input capture on timerB?

// ATtiny412 / ARDUINO
//                                     _____
//                             VDD   1|*    |20  GND
//       (TXD) (DAC) (AIN6) PA6  0   2|     |19  4~  PA3 (AIN3)(SCK)(EXTCLK)
//       (RXD)       (AIN7) PA7  1   3|     |18  5   PA0 (nRESET/UPDI)
// (MOSI)(TXD*)(SDA) (AIN1) PA1  2   4|_____|17  3   PA2 (AIN2)(MISO)(RXD*)(SCL)
//
//
// Compile with millis disabled (or millis on TCA0)
// Compile with 16MHz and the printed pulsewidth will be right.
// Connect output to servo to pin PA1
// Connect input from servotester or receiver to pin PA2
// Connect optional serial adapter RX to the default TX pin PA6 for debugging via serial

#define debug  //  comment or uncomment for serial print of servopulsewidth (uses extra flash)

#include <Servo_megaTinyCore.h>
Servo myservo;  // create servo object to control a servo

void setup() {
  takeOverTCD0();// This will give me control of timer TCD0.
#ifdef debug
  Serial.begin(115200);// Default Serial TX is on pin 0 (PA6)
#endif
  myservo.attach(2);  // attaches the servo pin to the servo object

  PORTA.PIN2CTRL = PORT_PULLUPEN_bm;

  /* configure event channel
     Connect pin PA2 to event channel 0 as event source for the channel 0
     Then connect both TCD0 events A and B (or called 0 and 1 in some places in the datasheet)
     as two users of the same event channel 0
  */
  EVSYS.ASYNCCH0   = EVSYS_ASYNCCH0_PORTA_PIN2_gc; //link event system async ch0 to pin A2
  EVSYS.ASYNCUSER6 = EVSYS_ASYNCUSER6_ASYNCCH0_gc;// Link TimerD Event0 (=ASYNCUSER6) to event ch0
  EVSYS.ASYNCUSER7 = EVSYS_ASYNCUSER7_ASYNCCH0_gc;// Link TimerD Event1 (=ASYNCUSER7) to event ch0

  /* configure timer D
     Both TCD0 events A and B are connected to the same pin via the channel 0 of the event system.
     Now we define EventA as the rising edge and event B as the falling edge of the pulse
     With the action "capture" this means that the TCD0 value will be capured into TCD0_CAPTUREA
     when a rising pulse edge occurs and in TCD0_CAPTUREB when a falling edge occurs.
     This will also set the interrupt flags in the INTFLAGS register of TCD0. So we can test
     that in the main loop, if a new capture is available.
  */
  TCD0.EVCTRLA = B01010101; // 7,6 CFG=filter on, 4 edge=rise , 2 action=capture, 0 input enable
  TCD0.EVCTRLB = B01000101; // 7,6 CFG=filter on, 4 edge=fall , 2 action=capture, 0 input enable

  TCD0.CTRLB |= TCD_WGMODE_ONERAMP_gc; // Set timer to one ramp mode (=default)
  TCD0.CTRLA |= TCD_CLKSEL_SYSCLK_gc; // Internal 16/20 MHz Oscillator (OSC20M)
  TCD0.CTRLA |= TCD_CNTPRES_DIV4_gc; // Division factor  (1, 4 or 32 possible)
  TCD0.CTRLA |= TCD_SYNCPRES_DIV4_gc; // Division factor  (1, 2, 4 or 8 possible)
  TCD0_CMPBCLR = 4095; // set TOP

  TCD0.CTRLA |= TCD_ENABLE_bm; // enable TCD0 (also sends a sync)
}

void loop() {
  uint16_t pulsewidth;
  if (TCD0.INTFLAGS & TCD_TRIGB_bm) {// test if a falling edge capture is present (so we have both edges)
    pulsewidth = (TCD0_CAPTUREB << 4) - (TCD0_CAPTUREA << 4);// cast 12 bit into 16 bit field and calc.
#ifdef debug
    Serial.println(pulsewidth >> 4); // cast back from 16 to 12 bits for printing
#endif
    //    TCD0.INTFLAGS |= TCD_TRIGB_bm | TCD_TRIGA_bm ;
    TCD0.INTFLAGS |= TCD_TRIGB_bm; // and clear the interrupt flag by writing a logical '1' to the bit
    myservo.writeMicroseconds(pulsewidth >> 4); // sets the servo position
  }
}
SpenceKonde commented 1 year ago

o_o

Well, I said basically nobody, not nobody (though I managed to spell basically wrong). That just means that there aren't enough people using it to invalidate approximations made by a model which assumes that such persons do not exist.

Besides, what you're doing doesn't count, because this issue is in reference to the type B timers. I'm pretty confident that fewer people are using the TCD than a TCB for input capture, so your're part of an even smaller subpopulation that does input capture with that monster. You're using a type D timer, because... well... why? It's not clear to me why you'd intentionally subject yourself to that - and in fact, your code manages to step around the pitfalls of the sync process, since you spend enough time doing other stuff after calling takeOverTCD0 that you can confidently write to the "static" * registers without checking to make sure you can (you cannot write zero to the enable bit and then in the very next clock cycle, write to a static register. You need to wait, I think for ENRDY in status, before you can write to static registers - but in your case the other stuff you've done since is easily going to take longer than the synchronizer ever would need unless you were driving it with a very slow external clock source or something wacky like that (the delay is on the order of a couple of sync clocks - it's probably X synchronizer clocks plus Y CLK_PER clocks - because it needs to sync the enable bit to the TCD domain to tell the timer to turn off (measured in synchronizer clocks), and it may take a couple of system clocks to sync the signal from the timer saying it's off, unlock the registers now - so the expectation would be for it to take a length of time made up of both kinds of clock, right?), where X and Y are numbers not larger than 3. I do not know X or Y, only that the total delay is longer than 2 system clocks, otherwise my _PROTECTED_WRITE to faultcontrol to turn pwm on or off would have been too slow to trip over it). Hell, the TCBs can count so high that you wouldn't need to prescale the clock... Plus in general the TCBs are a lot easier to work with because they aren't in a different clock domain so there's none of that syncing crap. Oh... duh... because you're already using the single TCB you get for servo.

* That's what they call it in the datasheet, in that chart of the types of registers. I'll note that the chart is flawed in it's characterization of the TCD0.STATUS register - because it's got 2 bits that work just like the bits in TCD0.INTFLAGS (which is apparently "normal", to the extent that intflags are ever normal) - in addition to the two read only ready bits that the authors clearly had in mind when classifying them. Frankly, I'm not sure that the decision to put the PWMACT bits in the STATUS register was a well-reasoned design decision in general (shouldn't they be in intflags? And shouldn't you be able to trigger interrupts with them?) but that's another matter.

hmeijdam commented 1 year ago

your code manages to step around the pitfalls of the sync process, since you spend enough time doing other stuff after calling takeOverTCD0 that you can confidently write to the "static" * registers without checking to make sure you can

Good to know that takeOverTCD0 is not waiting itself for the synchronizing to be done, before it returns. So if I would have placed it right above "configure timer D" I would have to add a "while" to check the ENRDY (or something) in the status reg.

And yes, servo was eating the other timer, so I decided to bite the TCD0 bullet.

SpenceKonde commented 1 year ago

Yeah, takeOverTCAn() guns the timer and resets it to POR state.

TCD has no such reset function

Maybe takeOverTCDn() should be takeOverTCDn(bool reset = 0) and if 'true' is passed, it will wait out the sync delay, then write 255 to every register with "flags" in it and then memset 0 to everything,

I feel like that's a little too hand-holdy tbh

SpenceKonde commented 1 year ago

Closing this issue. This feature is no longer explicitly planned, though some portions of it may be added to the core at some point.