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.

Sign up for GitHub

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

Jump to bottom

Software Buffer is overflowing instead of filling the hardware buffer #24

Open
Flole998 opened this issue Aug 30, 2021 · 7 comments
Open

Software Buffer is overflowing instead of filling the hardware buffer #24

Flole998 opened this issue Aug 30, 2021 · 7 comments

Comments

@Flole998
Copy link
Contributor

I think the current way of how the software/hardware buffers are handled is not ideal and I would suggest doing the following: If the software buffers are full do not simply drop the hardware buffers but instead allow them to fill up further by not reading them. I think this only requires a simple change:

acan2517FD/src/ACAN2517FD.cpp

Line 838 in f0b69f5

if ((it & (1 << 1)) != 0) { // Receive FIFO interrupt

Here a isFull check for the buffer needs to be appended. As the bits are for "interrupt pending" it shouldn't matter if we do read the register but don't fetch the data from the FIFO and the interrupt should still be pending. As the return value of isr_core is telling the calling code if it should run it again it is important that in this case "false" is returned so we are not deadlocking. I am not sure how well this goes with the attachInterrupt though as LOW would constantly re-trigger the isr as the pin is never rising, so maybe we would have to temporarily disable interrupts in that case and re-enable them once dispatchReceivedMessage has been called as then we definitely have space in the software buffer again.

Suggestions/concerns/ideas are welcome!
@pierremolinaro
Copy link
Owner

pierremolinaro commented Sep 2, 2021 via email

@Flole998
Copy link
Contributor Author

Flole998 commented Sep 2, 2021

Hi,

I have already done the modification myself. I will provide a diff for you as a reference when I get the chance.

Thanks!

@pierremolinaro
Copy link
Owner

pierremolinaro commented Sep 2, 2021 via email

@Flole998
Copy link
Contributor Author

Flole998 commented Sep 2, 2021

Got the diff pretty quick:

--- orig/src/ACAN2517FD.cpp     2021-09-02 14:19:42.368828645 +0200
+++ acan2517FD-master/src/ACAN2517FD.cpp        2021-09-02 14:23:03.186055488 +0200
@@ -507,12 +507,17 @@
       xTaskCreate (myESP32Task, "ACAN2517Handler", 1024, this, 256, NULL) ;
     #endif
     if (mINT != 255) { // 255 means interrupt is not used
+        isrDisabled = false;
       #ifdef ARDUINO_ARCH_ESP32
         attachInterrupt (itPin, inInterruptServiceRoutine, FALLING) ;
       #else
+        mSPI.usingInterrupt(itPin); // usingInterrupt is not implemented in Arduino ESP32
         attachInterrupt (itPin, inInterruptServiceRoutine, LOW) ; // Thank to Flole998
-        mSPI.usingInterrupt (itPin) ; // usingInterrupt is not implemented in Arduino ESP32
       #endif
+        interruptServiceRoutine = inInterruptServiceRoutine;
+    }
+    else {
+        isrDisabled = true;
     }

     /*
@@ -773,6 +778,15 @@
     if (NULL != callBackFunction) {
       callBackFunction (receivedMessage) ;
     }
+    if (mINT != 255 && isrDisabled) { // 255 means interrupt is not used
+        isrDisabled = false;
+#ifdef ARDUINO_ARCH_ESP32
+        attachInterrupt(digitalPinToInterrupt(mINT), interruptServiceRoutine, FALLING);
+#else
+        mSPI.usingInterrupt(digitalPinToInterrupt(mINT));
+        attachInterrupt(digitalPinToInterrupt(mINT), interruptServiceRoutine, LOW);
+#endif
+    }
   }
   return hasReceived ;
 }
@@ -836,8 +850,15 @@
     #endif
       const uint16_t it = readRegister16Assume_SPI_transaction (INT_REGISTER) ; // DS20005688B, page 34
       if ((it & (1 << 1)) != 0) { // Receive FIFO interrupt
-        receiveInterrupt () ;
-        handled = true ;
+          if (!mDriverReceiveBuffer.isFull()) {
+              receiveInterrupt();
+              handled = true;
+          }
+          else if (!isrDisabled) {
+              detachInterrupt(digitalPinToInterrupt(mINT));
+              mSPI.notUsingInterrupt(digitalPinToInterrupt(mINT));
+              isrDisabled = true;
+          }
       }
       if ((it & (1 << 10)) != 0) { // Transmit Attempt interrupt
       //--- Clear Pending Transmit Attempt interrupt bit
diff -ur orig/src/ACAN2517FD.h acan2517FD-master/src/ACAN2517FD.h
--- orig/src/ACAN2517FD.h       2021-09-02 14:19:42.368828645 +0200
+++ acan2517FD-master/src/ACAN2517FD.h  2021-08-30 15:11:20.730470600 +0200
@@ -134,6 +134,8 @@
   private: uint8_t mReceiveFIFOPayload ; // in byte count
   private: uint8_t mTXBWS_RequestedMode ;
   private: uint8_t mHardwareReceiveBufferOverflowCount ;
+  private: void (*interruptServiceRoutine) (void);
+  private: bool isrDisabled = true;

 //······················································································································
 //    Receive buffer
diff -ur orig/src/ACANFDBuffer.h acan2517FD-master/src/ACANFDBuffer.h
--- orig/src/ACANFDBuffer.h     2021-09-02 14:19:42.368828645 +0200
+++ acan2517FD-master/src/ACANFDBuffer.h        2021-08-30 15:43:47.408575000 +0200
@@ -54,6 +54,7 @@

   public: inline uint32_t size (void) const { return mSize ; }
   public: inline uint32_t count (void) const { return mCount ; }
+  public: inline bool isFull(void) const { return mCount == mSize; }
   public: inline uint32_t peakCount (void) const { return mPeakCount ; }

 //······················································································································

One possible issue with this though: The ESP-based CPUs are set to FALLING, but when the interrupt is re-attached it might be low already, so we should run the ISR after re-attaching the interrupt just to be sure (or check if the pin is low there).

I also changed the order of the mSPI.usingInterrupt as I think in this order it makes more sense as this way it is already set when the ISR is executed, otherwise the ISR might be fired right in that moment when it gets attached/activated and the usingInterrupt is executed too late (the ISR would be possibly running without it once).

@pierremolinaro
Copy link
Owner

pierremolinaro commented Sep 3, 2021 via email

@pierremolinaro
Copy link
Owner

pierremolinaro commented Sep 15, 2021 via email

@Flole998
Copy link
Contributor Author

That's probably a better approach. Did you verify if unmasking it causes an immediate interrupt when there's data pending? That's very important as otherwise there would be data stuck in the buffer.

@pierremolinaro pierremolinaro mentioned this issue Nov 13, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@Flole998 @pierremolinaro