Update kernel MRI to use mbed::UnbufferedSerial

This change includes:
* The DebugSerial constructor now only supports UART as the
  communication channel. It also only supports instantiation as a
  global object.
* Using mbed::UnbufferedSerial for UART communications instead of the
  higher level Arduino HardwareSerial classes means a few
  simplifications can be made:
  * No longer need to force execution of the serial begin() method from
    setup(). Can just construct the mbed::UnbufferedSerial object from
    DebugSerial's constructor.
  * Hooking UART interrupt to support CTRL+C is done in two stages:
    * First call UnbufferedSerial::attach() to configure UART for
      interrupts on received data.
    * Then call NVIC_SetVector() to replace mbed's ISR with the
      mriExceptionHandler assembly language routine.
* Now running DebugMonitor and UART interrupts at priority level 2.
  Priority level 1 is the default priority used by the STM USB driver.
  This allows the PC to have some basic communication with the USB
  stack while halted in the debugger.

Author: adamgreen

libraries/MRI/examples/MRI/MRI.ino

@@ -5,7 +5,7 @@
 
 #include <MRI.h>
 
-DebugSerial debugSerial(Serial1, USART1_IRQn, 230400);
+DebugSerial debugSerial(SERIAL1_TX, SERIAL1_RX, USART1_IRQn, 230400);
 
 void setup() {
 }

libraries/MRI/src/boards/portenta-h7/DebugSerial.cpp

@@ -12,7 +12,7 @@
    See the License for the specific language governing permissions and
    limitations under the License.
 */
-// GDB debugging of the Arduino Portenta-H7 over a serial connection.
+// GDB Kernel debugging of the Arduino Portenta-H7 over a serial UART connection.
 #include "DebugSerial.h"
 extern "C" {
     #include <core/mri.h>
@@ -55,26 +55,13 @@ static DebugSerial* g_pDebugSerial = NULL;
 // Will be setting initial breakpoint on setup() routine.
 void setup();
 
-// Forward Function Declarations
-static uint32_t readRegisterFromContextBuffer(Buffer* pBuffer, uint32_t regIndex);
-static void writeRegisterToContextBuffer(Buffer* pBuffer, uint32_t regIndex, uint32_t regValue);
-
+// The debugger uses this handler to catch faults, debug events, etc.
+extern "C" void mriExceptionHandler(void);
 
-DebugSerial::DebugSerial(HardwareSerial& serial, IRQn_Type irq, uint32_t baudRate, bool breakInSetup) :
-    _pContextBuffer(NULL), _commIsr(NULL), _serial(serial), _contextBufferSize(0), _baudRate(baudRate),
-    _irq(irq), _breakInSetup(breakInSetup)
-{
-    construct();
-}
 
-DebugSerial::DebugSerial(HardwareSerial& serial, IRQn_Type irq) :
-    _pContextBuffer(NULL), _commIsr(NULL), _serial(serial), _contextBufferSize(0), _baudRate(0),
-    _irq(irq), _breakInSetup(false)
+DebugSerial::DebugSerial(PinName txPin, PinName rxPin, IRQn_Type irq, uint32_t baudRate, bool breakInSetup /*=true*/) :
+    _serial(txPin, rxPin, baudRate), _irq(irq), _breakInSetup(breakInSetup)
 {
-    construct();
-}
-
-void DebugSerial::construct() {
     // Just return without doing anything if the singleton has already been initialized.
     // This ends up using the first initialized DebugSerial object.
     if (g_pDebugSerial != NULL) {
@@ -84,14 +71,10 @@ void DebugSerial::construct() {
 
     mriInit("");
 
-    if (_baudRate != 0) {
-        callSerialBeginFromSetup();
-    }
+    setupStopInSetup();
 }
 
-void DebugSerial::callSerialBeginFromSetup() {
-    _contextBufferSize = Platform_GetPacketBufferSize();
-    _pContextBuffer = new char[_contextBufferSize];
+void DebugSerial::setupStopInSetup() {
     mriCore_SetTempBreakpoint((uint32_t)setup, justEnteredSetupCallback, NULL);
 }
 
@@ -100,66 +83,13 @@ int DebugSerial::justEnteredSetupCallback(void* pv){
 }
 
 int DebugSerial::justEnteredSetup() {
-    Buffer buffer;
-
-    // Get current register context. Save away the contents of the registers in the context that we want to modify to
-    // setup for a call to g_pDebugSerial->initSerial().
-    Buffer_Init(&buffer, _pContextBuffer, _contextBufferSize);
-    Platform_CopyContextToBuffer(&buffer);
-    _lrOrig = readRegisterFromContextBuffer(&buffer, 14);
-    _pcOrig = readRegisterFromContextBuffer(&buffer, 15);
-
-    uint32_t lr = (uint32_t)setup;
-    uint32_t pc = (uint32_t)_initSerial;
-    writeRegisterToContextBuffer(&buffer, 14, lr);
-    writeRegisterToContextBuffer(&buffer, 15, pc);
-    Buffer_Reset(&buffer);
-    Platform_CopyContextFromBuffer(&buffer);
-
-    mriCore_SetTempBreakpoint((uint32_t)setup, justReturnedFromInitSerialCallback, NULL);
-
-    // Return 1 to indicate that we want to resume execution.
-    return 1;
-}
-
-static uint32_t readRegisterFromContextBuffer(Buffer* pBuffer, uint32_t regIndex) {
-    uint32_t regValue = 0;
-    uint8_t* pCurr = (uint8_t*)&regValue;
-
-    pBuffer->pCurrent = pBuffer->pStart + 8 * regIndex;
-    for (size_t i = 0 ; i < sizeof(regValue) ; i++) {
-        *pCurr++ = Buffer_ReadByteAsHex(pBuffer);
-    }
-    return regValue;
-}
-
-static void writeRegisterToContextBuffer(Buffer* pBuffer, uint32_t regIndex, uint32_t regValue) {
-    uint8_t* pCurr = (uint8_t*)&regValue;
-
-    pBuffer->pCurrent = pBuffer->pStart + 8 * regIndex;
-    for (size_t i = 0 ; i < sizeof(regValue) ; i++) {
-        Buffer_WriteByteAsHex(pBuffer, *pCurr++);
-    }
-}
-
-int DebugSerial::justReturnedFromInitSerialCallback(void* pv) {
-    return g_pDebugSerial->justReturnedFromInitSerial();
-}
-
-int DebugSerial::justReturnedFromInitSerial() {
-    Buffer buffer;
-
-    // Restore the registers to the way that they were before we called g_pDebugSerial->initSerial().
-    Buffer_Init(&buffer, _pContextBuffer, _contextBufferSize);
-    writeRegisterToContextBuffer(&buffer, 14, _lrOrig);
-    writeRegisterToContextBuffer(&buffer, 15, _pcOrig);
-    Buffer_Reset(&buffer);
-    Platform_CopyContextFromBuffer(&buffer);
+    initSerial();
 
     // Return 0 to indicate that we want to halt execution at the beginning of setup() or 1 to not force a halt.
     return _breakInSetup ? 0 : 1;
 }
 
+
 DebugSerial::~DebugSerial() {
     // IMPORTANT NOTE: You are attempting to destroy the connection to GDB which isn't allowed.
     //                 Don't allow your DebugSerial object to go out of scope like this.
@@ -168,56 +98,33 @@ DebugSerial::~DebugSerial() {
         // Loop forever.
     }
 }
+
 void DebugSerial::setSerialPriority(uint8_t priority) {
     mriCortexMSetPriority(_irq, priority, 0);
 }
 
-void DebugSerial::_initSerial() {
-    g_pDebugSerial->initSerial();
-    delay(STARTUP_DELAY_MSEC);
-}
-
 void DebugSerial::initSerial() {
-    _serial.begin(_baudRate);
-    setSerialPriority(0);
 
     // Hook communication port ISR to allow debug monitor to be awakened when GDB sends a command.
-    _commIsr = (IsrFunctionPtr) NVIC_GetVector(_irq);
-    NVIC_SetVector(_irq, (uint32_t)commInterruptHook);
-}
-
-void DebugSerial::commInterruptHook(void) {
-    g_pDebugSerial->pendDebugMonAndRunCommIsr();
-}
-
-void DebugSerial::pendDebugMonAndRunCommIsr(void) {
-    _commIsr();
-    if (_serial.available()) {
-        // Pend a halt into the debug monitor if there is now data from GDB ready to be read by it.
-        setMonitorPending();
-    }
+    _serial.attach(mriExceptionHandler);
+    setSerialPriority(2);
+    NVIC_SetVector(_irq, (uint32_t)mriExceptionHandler);
 }
 
 uint32_t DebugSerial::hasReceiveData(void) {
-    handleAnyPendingCommInterrupts();
-    return _serial.available();
-}
-
-void DebugSerial::handleAnyPendingCommInterrupts() {
-    if (NVIC_GetPendingIRQ(_irq)) {
-        _commIsr();
-        NVIC_ClearPendingIRQ(_irq);
-    }
+    return _serial.readable();
 }
 
 int DebugSerial::receiveChar(void) {
     while (!hasReceiveData()) {
     }
-    return _serial.read();
+    uint8_t byte;
+    _serial.read(&byte, 1);
+    return byte;
 }
 
 void DebugSerial::sendChar(int character) {
-    _serial.write(character);
+    _serial.write(&character, 1);
 }
 
 
@@ -227,9 +134,6 @@ void DebugSerial::sendChar(int character) {
 // Global Platform_* functions needed by MRI to initialize and communicate with MRI.
 // These functions will perform most of their work through the DebugSerial singleton.
 // ---------------------------------------------------------------------------------------------------------------------
-// The debugger uses this handler to catch faults, debug events, etc.
-extern "C" void mriExceptionHandler(void);
-
 struct SystemHandlerPriorities {
     uint8_t svcallPriority;
     uint8_t pendsvPriority;
@@ -244,19 +148,18 @@ static void switchFaultHandlersToDebugger();
 
 extern "C" void Platform_Init(Token* pParameterTokens) {
     SystemHandlerPriorities origPriorities = getSystemHandlerPrioritiesBeforeMriModifiesThem();
-    uint8_t                 debugMonPriority = 1;
+    uint8_t                 debugMonPriority = 2;
 
     __try
         mriCortexMInit((Token*)pParameterTokens, debugMonPriority, WAKEUP_PIN_IRQn);
     __catch
         __rethrow;
 
-    // UNDONE: Might want to always keep the USB handler at elevated priority.
-    // Set interrupt used by serial comms (UART or USB) at highest priority.
-    // Set DebugMonitor interrupt at next highest priority.
+    // USB defaults to a priority of 1, keep that as highest priority interrupt so that it can respond to PC requests
+    // while kernel debugging.
+    // Set interrupts used by UART serial comms and DebugMonitor at next highest priority.
     // Set all other external interrupts lower than both serial comms and DebugMonitor.
     restoreSystemHandlerPriorities(&origPriorities);
-    g_pDebugSerial->setSerialPriority(0);
 
     switchFaultHandlersToDebugger();
 }

libraries/MRI/src/boards/portenta-h7/DebugSerial.h

@@ -12,10 +12,11 @@
    See the License for the specific language governing permissions and
    limitations under the License.
 */
-// GDB debugging of the Arduino Portenta-H7 over a serial connection.
+// GDB Kernel debugging of the Arduino Portenta-H7 over a serial connection.
 #pragma once
 
 #include <Arduino.h>
+#include <mbed.h>
 
 extern "C" {
     typedef struct Token Token;
@@ -26,70 +27,40 @@ extern "C" {
 }
 
 
+// UNDONE: Should I put the debugger objects into the Arduino namespace?
 namespace arduino {
 
 class DebugSerial {
 public:
-    // You must declare your DebugSerial object as a global or function scoped static so that it doesn't get
-    // destroyed. Which constructor you use, depends on where you declare it.
-
-    // Use this constructor when declaring DebugSerial object as a global outside of any functions.
-    // You must specify the baudrate so that DebugSerial can call begin() on the specified serial object.
-    //  breakInSetup - should it break at beginning of setup().
-    //
-    // Global Example:
-    //      DebugSerial debugSerial(Serial1, USART1_IRQn, 115200);
-    DebugSerial(HardwareSerial& serial, IRQn_Type IRQn, uint32_t baudRate, bool breakInSetup=true);
-
-    // Use this constructor when declaring DebugSerial object as a function scoped static. You must call begin() on
-    // the serial object before constructing the DebugSerial object.
-    //
-    // Function Scoped Static Example:
-    //      #include <MRI.h>
-    //      void setup() {
-    //          Serial1.begin(115200);
-    //          static DebugSerial debugSerial(Serial1, USART1_IRQn);
-    //          __debugbreak();
-    //      }
-    DebugSerial(HardwareSerial& serial, IRQn_Type IRQn);
+    // You must declare your DebugSerial object as a global.
+    // Example:
+    //      DebugSerial debugSerial(SERIAL1_TX, SERIAL1_RX, USART1_IRQn, 230400);
+    DebugSerial(PinName txPin, PinName rxPin, IRQn_Type irq, uint32_t baudRate, bool breakInSetup=true);
 
     // You should never let your DebugSerial object go out of scope. Make it global or static. To warn you if you do
     // let it go out of scope by mistake, this destructor will break into GDB and then enter an infinite loop.
     ~DebugSerial();
 
 protected:
     void        construct();
-    void        callSerialBeginFromSetup();
+    void        setupStopInSetup();
 
     // These protected methods are called from global Platform* routines via singleton.
     void        setSerialPriority(uint8_t priority);
     uint32_t    hasReceiveData(void);
     int         receiveChar(void);
     void        sendChar(int character);
 
-    static void _initSerial();
     void        initSerial();
 
-    static void commInterruptHook(void);
-    void        pendDebugMonAndRunCommIsr();
-    void        handleAnyPendingCommInterrupts();
-
     static int  justEnteredSetupCallback(void* pvContext);
     int         justEnteredSetup();
-    static int  justReturnedFromInitSerialCallback(void* pvContext);
-    int         justReturnedFromInitSerial();
 
     typedef void(*IsrFunctionPtr)(void);
 
-    char*           _pContextBuffer;
-    IsrFunctionPtr  _commIsr;
-    HardwareSerial& _serial;
-    uint32_t        _contextBufferSize;
-    uint32_t        _baudRate;
-    uint32_t        _lrOrig;
-    uint32_t        _pcOrig;
-    IRQn_Type       _irq;
-    bool            _breakInSetup;
+    mbed::UnbufferedSerial  _serial;
+    IRQn_Type               _irq;
+    bool                    _breakInSetup;
 
     friend void     ::mriPlatform_Init(Token* pParameterTokens);
     friend uint32_t ::mriPlatform_CommHasReceiveData(void);