SpenceKonde / DxCore

Arduino core for AVR DA, DB, DD, EA and future DU-series parts - Microchip's latest and greatest AVRs. Library maintainers: Porting help and adviccee is available.
Other
189 stars 50 forks source link

Irratic behaviour? #177

Closed mrWheel closed 2 years ago

mrWheel commented 3 years ago

Hi,

I try to create a setup where a function is called every time a character comes in on Serial2.

I have code to show the behaviour I get (it is a simplified sketch)

/*
  Arduino-IDE settings for AVR128DB32:

    - Chip: "AVR128DB32"
    - Clock Speed: "24MHz internal"
    - millis()/micros() timer: "TCB2 (recomended)"
    - BOD level if enabled (Bootload burn req'd): "1.9V"
    - BOD Mode Active/Sleeping (Bootload burn req'd): "Disabled/Disabled"
    - Save EEPROM (Bootload burn req'd): "EEPROM retained"
    - Reset pin function (Bootload burn req'd): "Hardware Reset (recomended)"
    - Startup Time (Bootload burn req'd): "8ms"
    - Flash writing: "Disabled"
    - Attachinterrupt() Version: "On all pins, like usual"
    - Port: <select correct port>
    - Programmer:"jtag2updi"

    ==================================
    ==> DxCore version 1.3.8 <==
    ==================================
*/

#define _MAX_COMM_BUFF  100

char      commBuff[_MAX_COMM_BUFF] = {};
int       commBuffLen = 0;
uint32_t  waitTimer;
int32_t   x = 0;

const uint8_t RS485_XDIR  = PIN_PA2;  // RS485

//=============================================================
ISR(PORTF_PORT_vect) 
{
  //-- clear interrupt for RX2 (PIN_PF1)
  PORTF.PIN1CTRL &= ~(PORT_ISC_gm); // turn off interrupt
  // clear the flag too, otherwise it will fire the instant it's turned on.
  VPORTF.INTFLAGS |= 1<<1;

  Serial.println("\r\nPIN_PF1 ISR");
  Serial.flush();
  handleRS485Input();

  //-- re-enable interrupt on PIN_PF1 (Serial2 RX2):
  PORTF.PIN1CTRL = (PORTF.PIN1CTRL & ~(PORT_ISC_gm)) | PORT_ISC_BOTHEDGES_gc;

} //  ISR(portF)

//=============================================================
void handleRS485Input()
{
  int commBuffLen = 0;

  memset(commBuff, 0, _MAX_COMM_BUFF);
  waitTimer = millis()+1000;
  Serial.printf("\r\nwaitTimer[%u]->millis()[%u]->[%u]\r\n\n", waitTimer
                                                        , (int32_t)millis()
                                                        , (millis()-waitTimer));
  Serial.flush();

  //while ((int)(millis()-waitTimer) < 0) //-- why does this not 'work'?
  while (true) 
  {  
    x++;
    if ( (x%5000)==0 )   Serial.print('?');
    if ( (x%50000)==0 ) {Serial.println(); return;}
    while(Serial2.available())
    {
      commBuff[commBuffLen] = (char)Serial2.read();
      Serial.print(commBuff[commBuffLen]);
      Serial.flush();
      if (commBuff[commBuffLen] == '\n')
      {
        Serial.println("newLine");
        Serial.flush();
        commBuff[commBuffLen] = 0;
        //-- we'r done
        return;
      }
      commBuffLen++;
      commBuff[commBuffLen] = 0;
    }
  } //  while timer

} // handleRS485Input()

//=============================================================
void setup() 
{
  Serial.begin(9600);     //-- TXD=PA0, RXD=PA1
  while(!Serial) { delay(10); }
  Serial2.begin(19200);   //-- TXD=PF0, RXD=PF1
  while(!Serial2) { delay(10); }
  pinMode(PIN_PA2, OUTPUT);
  digitalWrite(PIN_PA2, LOW);

  Serial.println(F("\r\nTest RX2 interrupt on PIN_PF1\r\n"));
  Serial.println(F("And than it begins ...\r\n"));

  interrupts();
  //-- enable interrupt on PIN_PF1 (Serial2 RX2):
  PORTF.PIN1CTRL = (PORTF.PIN1CTRL & ~(PORT_ISC_gm)) | PORT_ISC_BOTHEDGES_gc;

} //  setup()

//=============================================================
void loop() 
{
  Serial.print('.');
  x++;
  if ((x%20000)==0)
  {
    Serial.println();
    Serial.flush();
  }
  delay(1000);

} //  loop();

"The other end (Controller)" is sending the string "STATUS" every few seconds.

This is the output:

Test RX2 interrupt on PIN_PF1
And than it begins ...
.......
PIN_PF1 ISR
waitTimer[7007]->millis()[0]->[6007]
??????????
...
PIN_PF1 ISR
waitTimer[10879]->millis()[0]->[9879]
ST
newLine
.......
PIN_PF1 ISR
waitTimer[17341]->millis()[0]->[16341]
ST
newLine
......
PIN_PF1 ISR
waitTimer[23802]->millis()[0]->[22802]
ST
newLine
.................................
PIN_PF1 ISR
waitTimer[56657]->millis()[0]->[55657]
ST
newLine
.......
PIN_PF1 ISR
waitTimer[63118]->millis()[0]->[62118]
ST
newLine
......
PIN_PF1 ISR
waitTimer[4043]->millis()[1]->[3043]
ST
newLine
.......
PIN_PF1 ISR
waitTimer[10506]->millis()[1]->[9506]
ST
newLine
......
PIN_PF1 ISR

waitTimer[16968]->millis()[1]->[15968]
ST
newLine
......
PIN_PF1 ISR

waitTimer[23429]->millis()[1]->[22429]

ST
newLine
.................................
PIN_PF1 ISR
waitTimer[56295]->millis()[1]->[55295]
ST
newLine
.......
PIN_PF1 ISR

waitTimer[62756]->millis()[1]->[61756]

ST
newLine

There are two things I really don't understand. 1) why is the output of millis() in this line zero and after a while 1 and after a long while 2 etc. :

  Serial.printf("\r\nwaitTimer[%u]->millis()[%u]->[%u]\r\n\n", waitTimer
                                                        , (int32_t)millis()
                                                        , (millis()-waitTimer));

2) why is the incomming string only 2 chars long with a new-line after ST???

I know that you shouldn't stay long in ISR's but for this project there is plenty of time between messages from the controller. If even possible, is there a better way to do this?

If firing by an incoming character is not possible, how can I setup a timer to call a function every, say, one or two seconds?

SpenceKonde commented 3 years ago
  1. No idea - but I also don't really know format strings... and we know that the value of millis isn't being treated as 0, because waitTimer is being set to millis() +1000 correctly
  2. Well, so wait what do you have? You've got it coming in on USART2, and right as data starts coming in, then you fire this very slow port interrupt. This takes so long to execute that by the time it's over, all the serial data has long since come and gone AND ONLY ONCE THAT INTERRUPT FINISHES DOES THE USART RX ISR HAVE A CHANCE TO RUN. That interrupt's job is to take the incoming characters from the USARTn.RXDATA register, and safely place them in the incoming serial buffer, hopefully fast enough that no data will be lost. Of course, that's already happened because another interrupt was blocking on Serial.flush() while the incoming data was passing it by.

And since RX is doublebuffered, it would get the first two bytes that were sent to it, and then probably the most recent character, a newline, which had been sitting in the incoming shift register when we finally got around to handling the serial RX ISR.

 //while ((int)(millis()-waitTimer) < 0) //-- why does this not 'work'?

Unsure what is meant here? I think it does "work" for these cases... however that is NOT how you are supposed to ever test timeouts, and other problems aside, parts of your code that do work now will collapse when millis rolls over every what, month and a half?).

The canonical way to test millis against timeouts is that you record the millis-value at the moment you're counting time since, and then subtract that value from the current value of millis, and then compare that with the length of time you want to wait, like this:

uint32_t start_time = millis();
while (millis() - start_time < 1000) ; //wait 1 second

I'm unsure of what the desired behavior is here....

Like, I'd go with one of the usual implementations, you pluck characters out of the buffer as they end up there, waiting until you see something that signifies the start of a command. Then each character you read gets put onto the end of a command buffer until you get something signfiying the end of a command. Then your command is in the command buffer, you decide what to do as a result of that command, and clear that buffer, rinse, repeat.

What is the purpose of the interrupt triggering off the RX pin? I mean I knew you had one that was used to wake from sleep, but that at the very most only needs to record the current time, maybe disable the interrupt, and clear the interrupt flag.

// clear the flag too, otherwise it will fire the instant it's turned on.

Is that the behavior? I would have expected worse, I'd expect the ISR to keep firing as soon as it returned if you didn't clear the flag, *even if you had disabled the port pin interrupt, because there's no place to enable or disable interrupt for a port in general, only the PINnCTRKL register's ISC bitfield.

mrWheel commented 3 years ago

Spence, thanks for your time (again)

First I want to complement you on the re-work of all the documentation on the DxCore. One request though: From an end-user perspective your documentation is far more readable than the specsheets from Microchip but what I miss are complete yet simple examples. I read your timer documentation but still I have no idea how to implement a simple timer to fire a function every few (milli)seconds. Same goes for the interrupt routines.

The canonical way to test millis against timeouts is that you record the millis-value at the moment you're counting time since, and then subtract that value from the current value of millis, and then compare that with the length of time you want to wait, like this:

uint32_t start_time = millis();
while (millis() - start_time < 1000) ; //wait 1 second

It just didn’t seem to work so I tried some variant. I will return to the “canonical way” tomorrow and report back although I now realize what I try to accomplish is not feasible.

What is the purpose of the interrupt triggering off the RX pin? I mean I knew you had one that was used to wake from sleep, but that at the very most only needs to record the current time, maybe disable the interrupt, and clear the interrupt flag.

I indeed copied the ISR from the example you gave me for waking the processor from sleep by pressing a key (the key pressed and indeed all the following chars are lost as the sleep/wakeup messes with the pulsetrain, as you explained). I thought that this code

  //-- clear interrupt for RX2 (PIN_PF1)
  PORTF.PIN1CTRL &= ~(PORT_ISC_gm); // turn off interrupt
  // clear the flag too, otherwise it will fire the instant it's turned on.
  VPORTF.INTFLAGS |= 1<<1;

Would clear the flag as the comment suggests..

The sketch is performing all kinds of stuff and in the mean time the “Controller” is sending questions (like “what is your STATUS) and commands (like “post data to a server”). Those questions and commands have a structured flow but can appear random. What is needed is a function that fires by incoming command or at more-or-less “fixed intervals”. At the moment I call the handleRS485Input() function on as many places of the code as I can but it would be much nicer if it is called by a timer or, what I tried in the example by an ISR.

SpenceKonde commented 3 years ago

Standard solution would be rto call handleRS485Input() in loop, and make sure that your code wasn't delaying, such that it would get back to loop() often enough.

mrWheel commented 3 years ago

Standard solution would be rto call handleRS485Input() in loop, and make sure that your code wasn't delaying, such that it would get back to loop() often enough.

Yes, handleRS485input() is called in loop() but that’s not enough as the challenge-response with the GSM-modem also needs a lot of attention and some responses take several seconds to return..

I have my hopes on some sort of a timer function now I understand ISR will not work.

SpenceKonde commented 3 years ago

I will say that more than anything else, what was screwing you in the code you posted the printing of all that shit within the ISR.

So there are calls to the GSM modem that block for seconds at a time - too long to for the response to the controller? I mean if there's a blocking call that takes longer to return than the maximum time you have to respond to a message from controller, it's theoretically impossible (ie, if you must respond to controller within 2 seconds, and a GSM modem call can take up to 3 seconds to return, there is no possible way to meet those constraints - on the other hand if you're waiting in a loop for something in the status of the GSM modem to change, that's a different matter. (it's not uncommon for there to be a function that is called during polling loops as well as from, say,the start of loop and such - on classic AVRs, for a time the cores had a weakly defined Yield() function that got called by delay between iterations. That apparently was cut from the API and has never showed up on modern AVRs. I think it appeared around the same time as that SerialEvent crap, and was lumped together with it in the "unfit for Arduino" pile and given the heave ho.

I indeed copied the ISR from the example you gave me for waking the processor from sleep by pressing a key (the key pressed and indeed all the following chars are lost as the sleep/wakeup messes with the pulsetrain, as you explained). I thought that this code Would clear the flag as the comment suggests..

Yeah, it does. But as long as you're still in the ISR, other interrupts can't fire.

mrWheel commented 3 years ago

.. on the other hand if you're waiting in a loop for something in the status of the GSM modem to change, that's a different matter. (it's not uncommon for there to be a function that is called during polling loops as well as from, say,the start of loop and such)

That is exactly the case. Of-course I call the handleRS485input() within that loop, but the code gets so cluttered with all those call’s!

Can (will) you give me an example code to setup a timer/prescaler/compare to fire a function?

SpenceKonde commented 3 years ago

The problem is that handleRS485input as written, cannot be run from an interrupt, as it will fail unless it is running outside of an interrupt, because it relies on Serial.available() and Serial.read() to see if there are characters and receive them. But no characters will be added to the buffer while the interrupt is executing! or you have already received and buffered the entire command. So I don't see how putting it into a timer ISR would make things any better, and no, I don't have any timer ISR implementations hanging around that I can go and show you (the correct timer to use is probably the RTC clocked from the internal 32kHz oscillator, the TCA's take out PWM, and the TCBs don't have flexible prescalers (options are /1, /2, and whatever TCA0 or TCA1 is using), and TCD0 is a byzantine nightmare to work with that took me over a year to realize that I could make PWM come out the right frequency when using it for millis on megaTinyCore). The only periodic interrupt I've implemented is the millis timer, because I've never been in a situation where it makes sense to do that. and currently I don't think you are in such a situation either.

How many potentially long busywait loops do you have that need the call to check for RS485 input? You've only listed 2, one waiting on the GSM modem, and one in loop, so calling the function(s) you need to in order to periodically

You also don't, in general, want tocall functions from an ISR (unless that's the only place they're called from, which so the compiler will just inline them. It's very inefficient because it has to save and restore every call-used register, whether or not the called function actually uses it (16 registers) in addition to the other ISR overhead.

mrWheel commented 3 years ago

Hi Spence, Reading your reply’s I began to realize that there is no way to “stop” only one interrupt. It is “all or nothing”. As an end-user it did not occur to me that handeling uarts is “just an interrupt implementation” as all others and not something magic that works all by itself. So at last I understand the interrupt-way is not the way to go ánd why you cannot run lengthy functions from within a ISR. I now call the handleRS485input() on strategic locations in the code and it seems to work like I planned. I learned a lot from your writings. Thanks for that!

SpenceKonde commented 2 years ago

Well, all interrupts can be disabled (well - the NMI (non-maskable interrupt can only be enabled, not disabled; it's generated when CRC integrity check is enabled and fails; once the NMI fires, if the NMI exexutes reti to leave the interrupt, it will immediately fire again, so that the chip is completely nonfunctional if it's code is corrupted. The CRC scan can be forced on by fuses, or turned on by software action. The core never uses it, but it is essential in life safety critical use cases. You're unlikely to go out for a drive if your car's breaks don't work at all because the antilock break controller is repeatedly executing that, even if the corruption only impacted code paths that were used only when you slammed on the breaks to avoid a collision. You'd want it to fail completely so you would be aware of it, right?)

The INTCTRL bitfield of every peripheral has bits that usually correspond to each of the interrupt flags, and an interrupt is triggered whenever both the flag and the interrupt enable bits are set, as long as the global interrupt flag is set and it's not currently running another interrupt. (unlike classic AVRs, the interrupt bit is not changes when entering and leaving an ISR - whether you're in an interrupt is tracked by CPUINT, and reflected in CPUINT.STATUS.

It is possible to set one (and only one) interrupt to be lvl1 priority (all other interrupts are lvl0. A lvl1 interrupt can fire even inside a level 0 interrupt.) This raises a number of additional considerations,)

But that approach doesn't seem appropriate in this use case