adafruit / RadioHead

A github'ified version of http://www.airspayce.com/mikem/arduino/RadioHead/
Other
186 stars 115 forks source link

Added RH_ASK support for SAMD51 processor #58

Open IoTPanic opened 3 years ago

IoTPanic commented 3 years ago

Fix to issue #28 which I will reclose once this has been at least tested working. I made a logical test with M4 ItsyBitsy to my change and the timer code works well, I just don't have a ASK radio to test it on so if someone could verify that would be great.

Issue

Issue was that compiling for RH_ASK failed due to commit #29 that remove the C definitions of RH_ASK since none of the library would compile without it.

Solution

I added the correct timer code for the SAMD51 along with IRQ handler

IoTPanic commented 3 years ago

Normally wouldn't open a PR if I didnt test but I'm hoping others with ASK radios will see it, right now there is no working RH_ASK to break for the SAMD51, and I validated the code I added works.

WunderBeard commented 3 years ago

@IoTPanic, I've tried ASK receiver testing to no avail. I ran the same code (based on the RadioHead ask_receiver example) on an Arduino UNO and the ItsyBitsy M4 Express. Using pin 11 for Rx. Used 5V to power the receiver in both cases. The UNO was able to receive, and the M4 was not. I used a 2:1 resistor voltage divider to protect the digital input of the M4; 2.5V should be enough to get a HIGH on a 3.3V input.

I was able to verify your new Arduino SAMD51 section of code was being called by inducing a compile error. I also verified that all code was executing normally except no bytes were ever received from the driver.recv(buf, &buflen) call (and buflen remains 60). I don't have an oscilloscope handy, but I verified with a voltmeter that the digital input is bouncing off of 0 volts every second as expected. (My transmitter test program just sends "Hello World!" every second.)

I'll continue to poke around a bit and perhaps get a transmitter hooked up and try the setup the other way around. Here's the receiver test code:

// ask_receiver_test
// Use RadioHead to receive messages from a simple ASK transmitter.
// Implements a simplex (one-way) ASK receiver.

#include <RH_ASK.h>
#include <SPI.h> // Not actually used but needed to compile

RH_ASK driver(2000, 11);    // 2000 bps, Rx on D11
// debug globals
bool failed = true;
bool gothere = false;

void setup()
{
  delay(2000);  // does not print startup messages without this delay
  Serial.begin(19200);
  if (driver.init()) {
    failed = false;
    Serial.println("init success");
  } else {
    failed = true;
    Serial.println("init failed");
  }
}

void loop()
{
  uint8_t buf[RH_ASK_MAX_MESSAGE_LEN];
  uint8_t buflen = sizeof(buf);

  if (driver.recv(buf, &buflen)) {
    gothere = true;
    if (buflen>2) {
      Serial.print(buflen);
      Serial.print(" bytes: ");
      for (int i=0; i<buflen-1; i++)
        Serial.print((char)buf[i]);
      Serial.println((char)buf[buflen-1]);
    }
  } else {
    Serial.println("got nothing");
  }

  delay(500);
  if (failed) {
    Serial.println("failed");
  } else {
    Serial.print("waiting ");
    Serial.println(buflen);
    if (gothere) {
      Serial.println("got there");
    }
  }
}

All I get is

init success
got nothing
waiting 60
got nothing
waiting 60
...
WunderBeard commented 3 years ago

@IoTPanic, I'm looking at your __SAMD51__ code, and I'm not understanding a few things.

You define RH_ASK_M4_TIMER and RH_ASK_M4_TIMER_IRQ, but never use them anywhere. Perhaps you meant to use RH_ASK_ZERO_TIMER instead of explicitly calling out TC3 on lines 248-267? Maybe RH_ASK_M4_TIMER_IRQ should be used on line 256: NVIC_EnableIRQ(TC3_IRQn); should be NVIC_EnableIRQ(RH_ASK_M4_TIMER_IRQ);. These changes would not change the functionality, so they are probably not an issue.

I'm not clear on how the M4 timers work, but just poking around I found examples of setting up TC3 using GCLK_PCHCTRL_GEN_GCLK1_Val instead of GCLK_PCHCTRL_GEN_GCLK0_Val (line 243). Is 0 the main CPU clock? This code is still a little over my head, but it's starting to sink in.

IoTPanic commented 3 years ago

@WunderBeard I started the fix by copying and pasting the M0 timer code since the two ICs are similar and RH_ASK_M4_TIMER and RH_ASK_M4_TIMER_IRQ are unused you are right, if they are left in there is nothing extra added to the binary but you would be correct in saying they can be removed or you can update the call to RH_ASK_M4_TIMER_IRQ.

GCLK0 is the main CPU clock, I chose this because in the past there has been issues with timer consistency in Arduino and if I used the CPU clock that is a known constant no matter what a users library setup is. I copied and pasted the code of this commit into a blank project, and tested that it works correctly. As long as you are using commit af749de it should work.

A good next step may be to make a new global variable within RH_ASK.cpp and increment it in the IRQ handler just to verify that the timer is running on your setup and to make sure the rate is correct. If you print it in loop once a second it should be quite close to the baud rate you are using.

WunderBeard commented 3 years ago

Yes, but doesn't the prescaler need to be divided by 8 to get 8 interrupts per bit (line 263)?

I can try to copy and paste to test the timer code independently (or try the global idea).

IoTPanic commented 3 years ago

@WunderBeard no, originally I used a by eight divider but in my testing the precision wasn't that good, in fact, I think the SAMD21 code needs to be updated to the same as I have it.

WunderBeard commented 3 years ago

@IoTPanic, I fixed it! I did the following changes:

 #elif defined (__arm__) && defined(__SAMD51__)
    // Arduino SAMD51
    #define RH_ASK_M4_TIMER TC3
    // Clock speed is 120MHz, prescaler of 64 provides good precision (~0.53 us ticks)
    #define RH_ASK_M4_PRESCALER 64
    #define RH_ASK_M4_TIMER_IRQ TC3_IRQn

    GCLK->PCHCTRL[TC3_GCLK_ID].reg = GCLK_PCHCTRL_GEN_GCLK0_Val |
                                   (1 << GCLK_PCHCTRL_CHEN_Pos);

    while (GCLK->SYNCBUSY.reg > 0);  // wait for sync

    RH_ASK_M4_TIMER->COUNT16.CTRLA.bit.ENABLE = 0;
    RH_ASK_M4_TIMER->COUNT16.WAVE.bit.WAVEGEN = TC_WAVE_WAVEGEN_MFRQ;
    while (RH_ASK_M4_TIMER->COUNT16.SYNCBUSY.reg != 0) {}  // wait for sync

    RH_ASK_M4_TIMER->COUNT16.INTENSET.reg = 0;
    RH_ASK_M4_TIMER->COUNT16.INTENSET.bit.MC0 = 1;

    // Enable IRQ
    NVIC_EnableIRQ(RH_ASK_M4_TIMER_IRQ);

    RH_ASK_M4_TIMER->COUNT16.CTRLA.reg &= ~0b011100000000;  // clear PRESCALER[2:0]
    while (RH_ASK_M4_TIMER->COUNT16.SYNCBUSY.reg != 0) {}  // wait for sync

    RH_ASK_M4_TIMER->COUNT16.CTRLA.reg |= (uint32_t)TC_CTRLA_PRESCALER_DIV64;  // must match RH_ASK_M4_PRESCALER
    while (RH_ASK_M4_TIMER->COUNT16.SYNCBUSY.reg != 0) {}  // wait for sync

    // Compute the count required to achieve the requested baud (with 8 interrupts per bit)
    uint32_t rc = (VARIANT_MCK / _speed) / RH_ASK_M4_PRESCALER / 8;
    RH_ASK_M4_TIMER->COUNT16.CC[0].reg = rc;
    while (RH_ASK_M4_TIMER->COUNT16.SYNCBUSY.reg != 0) {}  // wait for sync

    RH_ASK_M4_TIMER->COUNT16.CTRLA.bit.ENABLE = 1;
    while (RH_ASK_M4_TIMER->COUNT16.SYNCBUSY.reg != 0) {}  // wait for sync

I think DIV16 is an unnecessary amount of resolution (even though we have room in the counter for it). I think the fix was to divide by 8 so that you get the required 8 interrupts per bit for the configured bit rate.

Thanks for all of your help with this!

IoTPanic commented 3 years ago

@WunderBeard you were not using commit af749de RH_ASK_M4_TIMER->COUNT16.CTRLA.reg |= (uint32_t)TC_CTRLA_PRESCALER_DIV64; Looks to me like the 64 divider is being used which was my old broken code, could you please test my latest commit with the divide by 16?

WunderBeard commented 3 years ago

@IoTPanic, yes I was using your latest commit. What I'm saying is that I thought 16 was waaay too much resolution. Who needs 0.13 us resolution when 0.53 us will work just fine? So I changed it back to 64.

If you insist, it still works just fine if you plug in 16 as the prescaler.

The bug is on line 263. You must divide the ticks by 8 to get 8 interrupts per bit time.

IoTPanic commented 3 years ago

@WunderBeard I would like it to be kept to DIV16, but honestly it does not matter, if the prescaler is smaller its just more precision with no consequence. I just pushed a new commit, could you test that for me if you checkout (c73f900)? Thank you for repeating the You must divide the ticks by 8 to get 8 interrupts per bit time. I wasn't understanding because I thought the division was a function of the timer not the library and that didn't make sense to me.

IoTPanic commented 3 years ago

Awesome! Thank you so much for the help @WunderBeard!