Module: Mark some functions as virtual

[alistair23]

In an effort to support RadioLib on Tock (github.com/tock/tock) we want to replace some of the Module tranfer functions. This is because we want to replace the entire read/write command with an operating system call, instead of performing individual options. For example we don't want to manually control the CS line, but instead want to just specify a read buffer, write buffer and length and have Tock perform the operation for us.

The current Hal abstraction isn't powerful enough for this, so we need to override some Module functions instead.

[alistair23]

I have marked this as a draft as I don't yet have a complete list of the functions that I need to override. I wanted to open this though to get feedback if this is acceptable

[jgromes]

I'm not sure about this approach - why can't this be done on the HAL level? If you know that all your SPI transactions are syscalls, what is preventing you from impementing the syscall in the HAL?

[alistair23]

The problem is that we want to complete the operation in a single command.

So for example the current SPItransferStream() sends a byte at a time with this->hal->spiTransfer() and also receives a byte at a time with this->hal->spiTransfer() as well.

While the syscall interface instead requires us to complete the operation in a single command, something more like this:

    int16_t SPItransferStream(uint8_t* cmd, uint8_t cmdLen, bool write, uint8_t* dataOut, uint8_t* dataIn, size_t numBytes, bool waitForGpio, uint32_t timeout) override {
      char wbuf[cmdLen + numBytes];
      char rbuf[cmdLen + numBytes];
      uint8_t status = 0;

      for(uint8_t n = 0; n < cmdLen; n++) {
        wbuf[n] = cmd[n];
      }

      if (write) {
        for(uint8_t n = 0; n < numBytes; n++) {
          wbuf[cmdLen + n] = dataOut[n];
        }
      } else {
        for(uint8_t n = 0; n < numBytes; n++) {
          wbuf[cmdLen + n] = this->SPInopCommand;
        }
      }

      lora_phy_read_write_sync(wbuf, rbuf, cmdLen + numBytes);

      status = rbuf[cmdLen];
  
      if (!write) {
        for(size_t n = 0; n < numBytes; n++) {
          dataIn[n] = rbuf[cmdLen + 1 + n];
        }
      }

      return RADIOLIB_ERR_NONE;
    }

Which I don't think can be done with just the HAL

[jgromes]

I wasn't able to find source for lora_phy_read_write_sync() in tock, so I don't know how fixed is this implementation or if an alternative exists. If it doesn't, then I would instead propose to adapt HAL, not to try to override the Module methods. The reason is that if you do that, then you are esentially turning the Module class into another HAL layer, and that's not its purpose - the purpose is to contain functionality that is common for all the radio modules, it's not meant to implement "quirks" of the platform RadioLib is running on; that's why HAL exists as a separate class. It also leads to code duplication: for example, if at some point we find a bug somewhere in the Module implementations which you are overriding, you will have to port that fix into your own code.

The simplest change to HAL I can think of that would resolve this would be to add a "buffered" SPI transfer method to HAL as an alternative to the "byte-wise" transfer, and in Module, check whether it exists or not. If it does exist, Module can use it, it might even be preffered as it could be faster than going one byte at a time (e.g. if it is implemented by DMA). If it does not exist, then Module can fall back to byte-wise transfer.

My point is that I would rather adapt the existing HAL than create a new one ;)

[alistair23]

lora_phy_read_write_sync() is part of libtock-c. It's the C code that runs into user space that calls into the kernel.

I have basic functionality working and it seems that only SPItransferStream() needs to replaced. So a buffered SPI transfer method in the HAL would work as well.

Do you want me to prepare a PR to add a buffered transfer?

[jgromes]

It's going to take some changes in the current SPI methods in Module to not lose any functionality, so I will implement it and let you know. For now though, I will close this PR.

[jgromes]

@alistair23 in the end I ended up with implementing the buffered SPI in the HAL by default (the reasoning is that buffered SPI can be implemented by calling the byte-wise transfer in loop). So now you should be able to implement SPI transfer quite easily. I'm planning to release version 6.1.0 wiht this change later this week.

[alistair23]

Thanks for that!