Merge pull request #101 from geeksville/master

0.4.3 Fix #92: omg - for the last couple of weeks the official builds…

bin/build-all.sh

@@ -6,6 +6,10 @@ source bin/version.sh
 
 COUNTRIES="US EU433 EU865 CN JP"
 #COUNTRIES=US
+#COUNTRIES=CN
+
+BOARDS="ttgo-lora32-v2 ttgo-lora32-v1 tbeam heltec"
+#BOARDS=tbeam
 
 OUTDIR=release/latest
 
@@ -24,23 +28,21 @@ function do_build {
     SRCBIN=.pio/build/$ENV_NAME/firmware.bin
     SRCELF=.pio/build/$ENV_NAME/firmware.elf
     rm -f $SRCBIN 
-    pio run --environment $ENV_NAME # -v
+
+    # The shell vars the build tool expects to find
+    export HW_VERSION="1.0-$COUNTRY"
+    export APP_VERSION=$VERSION
+    export COUNTRY
+
+    pio run --jobs 4 --environment $ENV_NAME # -v
     cp $SRCBIN $OUTDIR/bins/firmware-$ENV_NAME-$COUNTRY-$VERSION.bin
     cp $SRCELF $OUTDIR/elfs/firmware-$ENV_NAME-$COUNTRY-$VERSION.elf
 }
 
 for COUNTRY in $COUNTRIES; do 
-
+    for BOARD in $BOARDS; do
-    HWVERSTR="1.0-$COUNTRY"
+        do_build $BOARD
-    COMMONOPTS="-DAPP_VERSION=$VERSION -DHW_VERSION_$COUNTRY -DHW_VERSION=$HWVERSTR -Wall -Wextra -Wno-missing-field-initializers -Isrc -Os -DAXP_DEBUG_PORT=Serial"
+    done
-
-    export PLATFORMIO_BUILD_FLAGS="$COMMONOPTS"
-
-    #do_build "tbeam0.7"
-    do_build "ttgo-lora32-v2"
-    do_build "ttgo-lora32-v1"
-    do_build "tbeam"
-    do_build "heltec"
 done
 
 # keep the bins in archive also

bin/version.sh

@@ -1,3 +1,3 @@
 
 
-export VERSION=0.4.2
+export VERSION=0.4.3

platformio.ini

@@ -12,9 +12,14 @@
 default_envs = tbeam
 
 [common]
-; default to a US frequency range, change it as needed for your region and hardware (CN, JP, EU433, EU865)
+; common is not currently used
-hw_version = US
 
+; REQUIRED environment variables - if not set the specified default will be sued
+; The following environment variables must be set in the shell if you'd like to override them.
+; They are used in this ini file as systenv.VARNAME, so in your shell do export "VARNAME=fish"
+; HW_VERSION (default US)
+; APP_VERSION (default emptystring)
+; HW_VERSION (default emptystring)
 
 [env]
 platform = espressif32
@@ -26,7 +31,11 @@ board_build.partitions = partition-table.csv
 
 ; note: we add src to our include search path so that lmic_project_config can override
 ; FIXME: fix lib/BluetoothOTA dependency back on src/ so we can remove -Isrc
-build_flags = -Wno-missing-field-initializers -Isrc -Isrc/rf95 -Os -Wl,-Map,.pio/build/output.map -DAXP_DEBUG_PORT=Serial -DHW_VERSION_${common.hw_version}
+build_flags = -Wno-missing-field-initializers -Isrc -Isrc/rf95 -Os -Wl,-Map,.pio/build/output.map 
+  -DAXP_DEBUG_PORT=Serial 
+  -DHW_VERSION_${sysenv.COUNTRY}
+  -DAPP_VERSION=${sysenv.APP_VERSION}
+  -DHW_VERSION=${sysenv.HW_VERSION}
 
 ; not needed included in ttgo-t-beam board file
 ; also to use PSRAM https://docs.platformio.org/en/latest/platforms/espressif32.html#external-ram-psram

src/MeshRadio.h

@@ -55,7 +55,10 @@
 #define CH_SPACING CH_SPACING_JP
 #define NUM_CHANNELS NUM_CHANNELS_JP
 #else
-#error "HW_VERSION not set"
+// HW version not set - assume US
+#define CH0 CH0_US
+#define CH_SPACING CH_SPACING_US
+#define NUM_CHANNELS NUM_CHANNELS_US
 #endif
 
 /**

src/configuration.h

@@ -31,8 +31,9 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 // If app version is not specified we assume we are not being invoked by the build script
 #ifndef APP_VERSION
-#define APP_VERSION 0.0.0   // this def normally comes from build-all.sh
+#error APP_VERSION, HW_VERSION, and HW_VERSION_countryname must be set by the build environment
-#define HW_VERSION 1.0 - US // normally comes from build-all.sh and contains the region code
+//#define APP_VERSION 0.0.0   // this def normally comes from build-all.sh
+//#define HW_VERSION 1.0 - US // normally comes from build-all.sh and contains the region code
 #endif
 
 // -----------------------------------------------------------------------------

src/main.cpp

@@ -336,9 +336,9 @@ void loop()
 {
     uint32_t msecstosleep = 1000 * 30; // How long can we sleep before we again need to service the main loop?
 
-    powerFSM.run_machine();
     gps.loop();
     router.loop();
+    powerFSM.run_machine();
     service.loop();
 
     ledPeriodic.loop();

src/rf95/CustomRF95.cpp

@@ -20,7 +20,7 @@ bool CustomRF95::canSleep()
 
     bool res = (_mode == RHModeInitialising || _mode == RHModeIdle || _mode == RHModeRx) && !isRx && txQueue.isEmpty();
     if (!res) // only print debug messages if we are vetoing sleep
-        DEBUG_MSG("canSleep, mode=%d, isRx=%d, txEmpty=%d, txGood=%d\n", _mode, isRx, txQueue.isEmpty(), _txGood);
+        DEBUG_MSG("radio wait to sleep, mode=%d, isRx=%d, txEmpty=%d, txGood=%d\n", _mode, isRx, txQueue.isEmpty(), _txGood);
 
     return res;
 }
@@ -78,13 +78,12 @@ void CustomRF95::handleInterrupt()
 {
     RH_RF95::handleInterrupt();
 
-    BaseType_t higherPriWoken = false;
     if (_mode == RHModeIdle) // We are now done sending or receiving
     {
         if (sendingPacket) // Were we sending?
         {
             // We are done sending that packet, release it
-            packetPool.releaseFromISR(sendingPacket, &higherPriWoken);
+            packetPool.release(sendingPacket);
             sendingPacket = NULL;
             // DEBUG_MSG("Done with send\n");
         }
@@ -123,43 +122,35 @@ void CustomRF95::handleInterrupt()
             }
 
             if (!pb_decode_from_bytes(payload, payloadLen, SubPacket_fields, p)) {
-                packetPool.releaseFromISR(mp, &higherPriWoken);
+                packetPool.release(mp);
             } else {
                 // parsing was successful, queue for our recipient
                 mp->has_payload = true;
 
-                deliverToReceiverISR(mp, &higherPriWoken);
+                deliverToReceiver(mp);
             }
 
             clearRxBuf(); // This message accepted and cleared
         }
 
-        higherPriWoken |= handleIdleISR();
+        handleIdleISR();
     }
-
-    // If we call this _IT WILL NOT RETURN_
-    if (higherPriWoken)
-        portYIELD_FROM_ISR();
 }
 
 /** The ISR doesn't have any good work to do, give a new assignment.
  *
  * Return true if a higher pri task has woken
  */
-bool CustomRF95::handleIdleISR()
+void CustomRF95::handleIdleISR()
 {
-    BaseType_t higherPriWoken = false;
-
     // First send any outgoing packets we have ready
-    MeshPacket *txp = txQueue.dequeuePtrFromISR(0);
+    MeshPacket *txp = txQueue.dequeuePtr(0);
     if (txp)
         startSend(txp);
     else {
         // Nothing to send, let's switch back to receive mode
         setModeRx();
     }
-
-    return higherPriWoken;
 }
 
 /// This routine might be called either from user space or ISR
@@ -197,6 +188,8 @@ void CustomRF95::startSend(MeshPacket *txp)
 
 void CustomRF95::loop()
 {
+    RH_RF95::loop();
+
     // It should never take us more than 30 secs to send a packet, if it does, we have a bug, FIXME, move most of this
     // into CustomRF95
     uint32_t now = millis();

src/rf95/CustomRF95.h

@@ -52,5 +52,5 @@ class CustomRF95 : public RH_RF95, public RadioInterface
     void startSend(MeshPacket *txp);
 
     /// Return true if a higher pri task has woken
-    bool handleIdleISR();
+    void handleIdleISR();
 };

src/rf95/RH_RF95.cpp

@@ -34,16 +34,12 @@ bool RH_RF95::init()
     if (!RHSPIDriver::init())
         return false;
 
-    // Determine the interrupt number that corresponds to the interruptPin
-    int interruptNumber = digitalPinToInterrupt(_interruptPin);
-    if (interruptNumber == NOT_AN_INTERRUPT)
-        return false;
 #ifdef RH_ATTACHINTERRUPT_TAKES_PIN_NUMBER
     interruptNumber = _interruptPin;
 #endif
 
     // Tell the low level SPI interface we will use SPI within this interrupt
-    spiUsingInterrupt(interruptNumber);
+    // spiUsingInterrupt(interruptNumber);
 
     // No way to check the device type :-(
 
@@ -114,18 +110,35 @@ bool RH_RF95::init()
             return false; // Too many devices, not enough interrupt vectors
     }
     _deviceForInterrupt[_myInterruptIndex] = this;
+
+    return enableInterrupt();
+}
+
+bool RH_RF95::enableInterrupt()
+{
+    // Determine the interrupt number that corresponds to the interruptPin
+    int interruptNumber = digitalPinToInterrupt(_interruptPin);
+    if (interruptNumber == NOT_AN_INTERRUPT)
+        return false;
+
     if (_myInterruptIndex == 0)
-        attachInterrupt(interruptNumber, isr0, RISING);
+        attachInterrupt(interruptNumber, isr0, ONHIGH);
     else if (_myInterruptIndex == 1)
-        attachInterrupt(interruptNumber, isr1, RISING);
+        attachInterrupt(interruptNumber, isr1, ONHIGH);
     else if (_myInterruptIndex == 2)
-        attachInterrupt(interruptNumber, isr2, RISING);
+        attachInterrupt(interruptNumber, isr2, ONHIGH);
     else
         return false; // Too many devices, not enough interrupt vectors
 
     return true;
 }
 
+void RH_INTERRUPT_ATTR RH_RF95::disableInterrupt()
+{
+    int interruptNumber = digitalPinToInterrupt(_interruptPin);
+    detachInterrupt(interruptNumber);
+}
+
 void RH_RF95::prepareDeepSleep()
 {
     // Determine the interrupt number that corresponds to the interruptPin
@@ -143,6 +156,13 @@ bool RH_RF95::isReceiving()
                                         RH_RF95_MODEM_STATUS_HEADER_INFO_VALID)) != 0;
 }
 
+void RH_INTERRUPT_ATTR RH_RF95::handleInterruptLevel0()
+{
+    disableInterrupt(); // Disable our interrupt until our helper thread can run (because the IRQ will remain asserted until we
+                        // talk to it via SPI)
+    pendingInterrupt = true;
+}
+
 // C++ level interrupt handler for this instance
 // LORA is unusual in that it has several interrupt lines, and not a single, combined one.
 // On MiniWirelessLoRa, only one of the several interrupt lines (DI0) from the RFM95 is usefuly
@@ -153,25 +173,29 @@ void RH_RF95::handleInterrupt()
     // Read the interrupt register
     uint8_t irq_flags = spiRead(RH_RF95_REG_12_IRQ_FLAGS);
 
+    // ack all interrupts
+    // note from radiohead author wrt old code (with IMO wrong fix)
+    // Sigh: on some processors, for some unknown reason, doing this only once does not actually
+    // clear the radio's interrupt flag. So we do it twice. Why? (kevinh - I think the root cause we want level
+    // triggered interrupts here - not edge.  Because edge allows us to miss handling secondard interrupts that occurred
+    // while this ISR was running.  Better to instead, configure the interrupts as level triggered and clear pending
+    // at the _beginning_ of the ISR.  If any interrupts occur while handling the ISR, the signal will remain asserted and
+    // our ISR will be reinvoked to handle that case)
+    spiWrite(RH_RF95_REG_12_IRQ_FLAGS, 0xff); // Clear all IRQ flags
+
     // Note: there can be substantial latency between ISR assertion and this function being run, therefore
     // multiple flags might be set.  Handle them all
 
     // Note: we are running the chip in continuous receive mode (currently, so RX_TIMEOUT shouldn't ever occur)
     bool haveRxError = irq_flags & (RH_RF95_RX_TIMEOUT | RH_RF95_PAYLOAD_CRC_ERROR);
-    if (haveRxError)
+    if (haveRxError) {
-    //    if (_mode == RHModeRx && irq_flags & (RH_RF95_RX_TIMEOUT | RH_RF95_PAYLOAD_CRC_ERROR))
-    {
         _rxBad++;
         clearRxBuf();
-    }
+    } else if (irq_flags & RH_RF95_RX_DONE) {
-
-    if ((irq_flags & RH_RF95_RX_DONE) && !haveRxError) {
         // Read the RegHopChannel register to check if CRC presence is signalled
         // in the header. If not it might be a stray (noise) packet.*
         uint8_t crc_present = spiRead(RH_RF95_REG_1C_HOP_CHANNEL) & RH_RF95_RX_PAYLOAD_CRC_IS_ON;
 
-        spiWrite(RH_RF95_REG_12_IRQ_FLAGS, 0xff); // Clear all IRQ flags, required before reading fifo (according to datasheet)
-
         if (!crc_present) {
             _rxBad++;
             clearRxBuf();
@@ -219,13 +243,14 @@ void RH_RF95::handleInterrupt()
         setModeIdle();
     }
 
-    // ack all interrupts, note - we did this already in the RX_DONE case above, and we don't want to do it twice
+    enableInterrupt(); // Let ISR run again
-    if (!(irq_flags & RH_RF95_RX_DONE)) {
+}
-        // Sigh: on some processors, for some unknown reason, doing this only once does not actually
+
-        // clear the radio's interrupt flag. So we do it twice. Why?
+void RH_RF95::loop()
-        // kevinh: turn this off until root cause is known, because it can cause missed interrupts!
+{
-        // spiWrite(RH_RF95_REG_12_IRQ_FLAGS, 0xff); // Clear all IRQ flags
+    while (pendingInterrupt) {
-        spiWrite(RH_RF95_REG_12_IRQ_FLAGS, 0xff); // Clear all IRQ flags
+        pendingInterrupt = false; // If the flag was set, it is _guaranteed_ the ISR won't be running, because it masked itself
+        handleInterrupt();
     }
 }
 
@@ -235,17 +260,17 @@ void RH_RF95::handleInterrupt()
 void RH_INTERRUPT_ATTR RH_RF95::isr0()
 {
     if (_deviceForInterrupt[0])
-        _deviceForInterrupt[0]->handleInterrupt();
+        _deviceForInterrupt[0]->handleInterruptLevel0();
 }
 void RH_INTERRUPT_ATTR RH_RF95::isr1()
 {
     if (_deviceForInterrupt[1])
-        _deviceForInterrupt[1]->handleInterrupt();
+        _deviceForInterrupt[1]->handleInterruptLevel0();
 }
 void RH_INTERRUPT_ATTR RH_RF95::isr2()
 {
     if (_deviceForInterrupt[2])
-        _deviceForInterrupt[2]->handleInterrupt();
+        _deviceForInterrupt[2]->handleInterruptLevel0();
 }
 
 // Check whether the latest received message is complete and uncorrupted

src/rf95/RH_RF95.h

@@ -806,12 +806,17 @@ class RH_RF95 : public RHSPIDriver
     /// Return true if we are currently receiving a packet
     bool isReceiving();
 
+    void loop(); // Perform idle processing
+
   protected:
     /// This is a low level function to handle the interrupts for one instance of RH_RF95.
     /// Called automatically by isr*()
     /// Should not need to be called by user code.
     virtual void handleInterrupt();
 
+    /// This is the only code called in ISR context, it just queues up our helper thread to run handleInterrupt();
+    void RH_INTERRUPT_ATTR handleInterruptLevel0();
+
     /// Examine the revceive buffer to determine whether the message is for this node
     void validateRxBuf();
 
@@ -846,6 +851,11 @@ class RH_RF95 : public RHSPIDriver
     /// Index of next interrupt number to use in _deviceForInterrupt
     static uint8_t _interruptCount;
 
+    bool enableInterrupt();  // enable our IRQ
+    void disableInterrupt(); // disable our IRQ
+
+    volatile bool pendingInterrupt = false;
+    
     /// The configured interrupt pin connected to this instance
     uint8_t _interruptPin;
 

src/rf95/RadioInterface.cpp

@@ -15,8 +15,8 @@ ErrorCode SimRadio::send(MeshPacket *p)
     return ERRNO_OK;
 }
 
-void RadioInterface::deliverToReceiverISR(MeshPacket *p, BaseType_t *higherPriWoken)
+void RadioInterface::deliverToReceiver(MeshPacket *p)
 {
     assert(rxDest);
-    assert(rxDest->enqueueFromISR(p, higherPriWoken)); // NOWAIT - fixme, if queue is full, delete older messages
+    assert(rxDest->enqueue(p, 0)); // NOWAIT - fixme, if queue is full, delete older messages
 }

src/rf95/RadioInterface.h

@@ -24,7 +24,7 @@ class RadioInterface
     /**
      * Enqueue a received packet for the registered receiver
      */
-    void deliverToReceiverISR(MeshPacket *p, BaseType_t *higherPriWoken);
+    void deliverToReceiver(MeshPacket *p);
 
   public:
     /** pool is the pool we will alloc our rx packets from

src/sleep.cpp

@@ -135,7 +135,7 @@ static void waitEnterSleep()
 
     uint32_t now = millis();
     while (!doPreflightSleep()) {
-        delay(10); // Kinda yucky - wait until radio says say we can shutdown (finished in process sends/receives)
+        delay(100); // Kinda yucky - wait until radio says say we can shutdown (finished in process sends/receives)
 
         if (millis() - now > 30 * 1000) { // If we wait too long just report an error and go to sleep
             recordCriticalError(ErrSleepEnterWait);