Looks ok to me. I had not realized that actual pins were involved (before I thought that some virtual dummy pins were added just to signal to the SPI code that SUBGHZ_SPI was to be used), but now I see that there are actual pins attached to the SUBGHZ_SPI module. IICU these are not regular SPI pins (in particular MISO seems to be output-only), so I do not think SUBGHZ_SPI can be used to drive an external SPI slave, right?

I do wonder if the API offered now (passing these debug pins to select SUBGHZ_SPI, but by default not actually using those pins but only use them when calling debug_SubGHzSPI_pins) is a bit unexpected, but it does seem pragmatic. One alternative I could imagine is to add an SPI constructor that just accepts the SUBGHZ_SPI pointer instead of pin numbers (and passes that on to spi_init by presetting _spi->spi and keeping the pin members of _spi set to NC), then it is a bit clearer that no pins are involved (and then to configure the debug out pins, you could pass the pins as normal maybe, even also support passing a subset). Not sure if that would be much better or worth the work, though.

Hi @matthijskooijman

This PR is not ready as I want made some tests.
You are right about the pins they are for debug purpose only so should be only output.

@matthijskooijman

Below is my 'gist' on how to make the SUBGHZSPI radio working with basicmac library:

#ifdef HAL_SUBGHZ_MODULE_ENABLED

#define SUBGHZ_DEFAULT_TIMEOUT     100U    /* HAL Timeout in ms               */
/* SystemCoreClock dividers. Corresponding to time execution of while loop.   */
#define SUBGHZ_DEFAULT_LOOP_TIME   ((SystemCoreClock*28U)>>19U)

static SUBGHZ_HandleTypeDef hsubghz = {.Init = {.BaudratePrescaler =
                                               SUBGHZSPI_BAUDRATEPRESCALER_8 } };
#endif /* HAL_SUBGHZ_MODULE_ENABLED */

static void hal_spi_init () {
#ifdef HAL_SUBGHZ_MODULE_ENABLED
    HAL_SUBGHZ_Init(&hsubghz);
#endif /* HAL_SUBGHZ_MODULE_ENABLED */
}

void hal_spi_select (int on) {
    if (on)
        LL_PWR_SelectSUBGHZSPI_NSS();
    else
        LL_PWR_UnselectSUBGHZSPI_NSS();
}

// perform SPI transaction with radio
u1_t hal_spi (u1_t out) {
#ifdef HAL_SUBGHZ_MODULE_ENABLED
  HAL_StatusTypeDef status = HAL_OK;
  __IO uint32_t count;

  count = SUBGHZ_DEFAULT_TIMEOUT * SUBGHZ_DEFAULT_LOOP_TIME;

  /* Wait until TXE flag is set */
  do
  {
    if (count == 0U)
    {
      status = HAL_ERROR;
      hsubghz.ErrorCode = HAL_SUBGHZ_ERROR_TIMEOUT;
      break;
    }
    count--;
  } while (READ_BIT(SUBGHZSPI->SR, SPI_SR_TXE) != (SPI_SR_TXE));

  __IO uint8_t *spidr = ((__IO uint8_t *)&SUBGHZSPI->DR);
  *spidr = out;

  count = SUBGHZ_DEFAULT_TIMEOUT * SUBGHZ_DEFAULT_LOOP_TIME;

  /* Wait until RXNE flag is set */
  do
  {
    if (count == 0U)
    {
      status = HAL_ERROR;
      hsubghz.ErrorCode = HAL_SUBGHZ_ERROR_TIMEOUT;
      break;
    }
    count--;
  } while (READ_BIT(SUBGHZSPI->SR, SPI_SR_RXNE) != (SPI_SR_RXNE));

  return (uint8_t)(READ_REG(SUBGHZSPI->DR));
#else
  return 0;
#endif /* HAL_SUBGHZ_MODULE_ENABLED */
}

See this reference for more details.

Below is my 'gist' on how to make the SUBGHZSPI radio working with basicmac library:

Hey, cool! Once this PR is merged, I'd be interested in merging your work into basicmac (where I happen to also have write access ;-P).

I'm also wondering what pin number is a sketch supposed to pass to various begin/transfer functions? Currently it can be any non-zero number, but that seems a bit broad (but if so, it should probably be at least documented in a comment on the class)?

It is documented here:
https://github.com/stm32duino/wiki/wiki/API#spi

Mainly:

We give to the user 3 possibilities about the management of the CS pin:

the CS pin is managed directly by the user code before to transfer the data (like the Arduino SPI library)
or the user gives the CS pin number to the library API and the library manages itself the CS pin (see example below)
or the user uses a hardware CS pin linked to the SPI peripheral

With SubGHZSPI, 2 last cases are the same as there is no hardware CS.

Hello @fpistm any advice on using it to communicate with the SubGHZ IP?
The result of the command 0x1D 07 40 00 00 00 should be chip status and the default LoRa sync word, ie: 0x00 00 00 00 14 24. But I get 0xFF FF FF FF 00 00

#include <SPI.h>

#define SubGHZ_NOP 0x00
#define SubGHZ_READ_REGISTER 0x1D
#define SubGHZ_SIZE_READ_REGISTER 4
#define SubGHZ_REG_LR_SYNCWORD_MSB 0x07
#define SubGHZ_REG_LR_SYNCWORD_LSB 0x40
#define SubGHZ_SIZE_REG_LR_SYNCWORD 2


void setup() {
  Serial.begin(921600);
  delay(1000);
  
  SubGHZ_SPI.begin(!CS_PIN_CONTROLLED_BY_USER);
  uint8_t buffer[SubGHZ_SIZE_READ_REGISTER + SubGHZ_SIZE_REG_LR_SYNCWORD] = {SubGHZ_READ_REGISTER, SubGHZ_REG_LR_SYNCWORD_MSB, SubGHZ_REG_LR_SYNCWORD_LSB, SubGHZ_NOP, SubGHZ_NOP, SubGHZ_NOP};

  Serial.print("Command: 0x");
  for (uint8_t i; i<SubGHZ_SIZE_READ_REGISTER + SubGHZ_SIZE_REG_LR_SYNCWORD; i++) {
    Serial.printf("%.2X ", buffer[i]);
  }
  Serial.println();

  Serial.print("Result: 0x");
  SubGHZ_SPI.transfer(!CS_PIN_CONTROLLED_BY_USER, buffer, 4);
  for (int i; i<SubGHZ_SIZE_READ_REGISTER + SubGHZ_SIZE_REG_LR_SYNCWORD; i++) {
    Serial.printf("%.2X ", buffer[i]);
  }
  Serial.println();
}

void loop() {}

Sorry if it is a basic question, last time I used Arduino SPI was a while ago...

@matthijskooijman long time no see :) Glad that you are still playing with LoRa!

Hi @Oliv4945
This PR allows to manage the SUBGHZSPI not the 'chip'. It requires to be enabled properly but it is not the goal of this PR.

It is documented here:
https://github.com/stm32duino/wiki/wiki/API#spi

Hm, I'm not sure I understand entirely yet, so this post is me trying to make sense of things. Note that I personally do not really care about letting the SPI library control the CS pin, since it offers less control and clarity about when the CS pin is asserted exactly (and it seems this is also inconsistent or bugged currently, see below). However, if we offer this feature, at least it should be consistent.

1. the CS pin is managed directly by the user code before to transfer the data (like the Arduino SPI library)

In regular SPI library, this is done by not passing a CS pin to the constructor, and then not passing any pin numbers to the begin/begintransaction/transfer functions (or passing CS_CONTROLLED_BY_USER).

2. or the user gives the CS pin number to the library API and the library manages itself the CS pin (see example below)

In regular SPI library, this is done by not passing a CS pin to the constructor, and passing the pin to be controlled to begin/begintransaction/transfer functions.

3. or the user uses a hardware CS pin linked to the SPI peripheral

In regular SPI library, this is done by passing a CS pin to the constructor. The pin value passed to begin/begintransaction/transfer is ignored.

For SubGHZ_SPI, case 1 (from the user perspective) ends up passing DEBUG_SUBGHZSPI_NSS to the regular SPI library, which makes it look like case 3, except that some subghz-specific exceptions in spi_init prevent pinmuxing the pins and disable SPI hardware NSS control.

For SubGHZ_SPI, case 2 (from the user perspective), this ends up controlling the NSS virtual pin, regardless of which pin is passed. This seems a bit confusing to me, it would make sense if this only happened when passing DEBUG_SUBGHZSPI_NSS or maybe some other value?

For SubGHZ_SPI, case 3 (from the user perspective) ends up behaving almost exactly like case 1 (since there DEBUG_SUBGHZSPI_NSS is also passed to the plain SPI library, and the same exceptions in spi_init will trigger), except that the pin value passed to begin/begintransaction/transfer is not ignored but instead causes the internal NSS signal to be controlled.

It seems this interface is a bit confusing, though I'm not sure what would be better. From the user perspective, I guess there's basically two cases: Where the use controls the internal NSS signal manually (which works fine as case 1 now), and where the SPI library automatically controls it (which I guess feels more like case 3 than case 2, except that I think the timing of NSS switches is more like case 2 than case 3). Maybe the API to trigger automatic NSS control should be neither case 2 or 3, but be some SubGHZ_SPI-specific API?

I also wonder if it makes sense to have a SubGHZ_SPI constructor that accepts pins at all, though. Normally, the pins passed are passed through the pin maps to be checked, so you can only pass pins that are actually connected to the SPI block. However, for SubGHZ_SPI, this pin map is skipped (because of the spi_init exception), so any pins passed will work (in the sense that the SPI block controls the internal SPI signals, not the pins passed). Only when calling enableDebugPins() will the pins be applied to the pinmap (but any failures by incorrect pins are, I think, ignored). I guess this constructor could be useful when using enableDebugPins() to select to which pins the debug signals should be mapped exactly, except that they can only be mapped in exactly one place, so this does not add anything.

So maybe the SUBGHZSPIClass constructor with arguments can just be removed? This also makes (maybe) it a little more clear that case 3 (hardware control of NSS) is not supported (though I guess directly controlling the internal NSS signal might be more similar to case 3 than case 2, maybe...).

NSS hardware control bugged?

This is probably off-topic, but I noticed this in the code and want to at least mention this. When using NSS hardware control (case 3), according to the reference manual the NSS line is asserted when the SPI unit is enabled (SPE is set). Looking at the code, this is done when SPI.begin() is called, and it remains set until SPI.end() is called, leaving the NSS pin asserted all the time. I would expect that NSS is only asserted between beginTransaction() and endTransaction(), but maybe this code and/or API predates the presence of beginTransaction() and endTransaction() (which were added later mostly to allow switching between settings and to allow auto-disabling interrupts during transactions, I'm not sure if we considered hardware NSS control when adding these since this happened for AVR initially) and calling end() between transactions is how this is intended to be used?

There also seems to be a discrepancy between case 2 and 3 in terms of timing: For case 2, the NSS pin is asserted for each transfer separately (which can be each byte, every 2 bytes for transfer16 or complete transactions if a complete buffer is passed to transfer), while for case 3, the NSS pin is asserted for longer.

Finally, case 3 is now selected by the constructor, so you cannot mix that with other cases (e.g. have one slave where the NSS is controlled by hardware and another where the NSS is controlled manually), but I guess that is fine, since then you can just as well control everything manually (except when mixing libraries using the same SPI bus with different expectations, but I suspect most if not all libraries implement case 1 anyway).

Arduino API

Another thing to wonder is what the official Arduino APIs for this are. I haven't checked yet, but I'm pretty sure AVR only supports case 1 (with the same API). Checking SAMD I seem that is also case 1 only, but SAM supports hardware NSS control (but the API is more like case 2 rather than case 3, since a single SPI block has multiple channels/NSS pins it can control). I haven't looked into detail to when NSS is asserted, but I suspect that there might also be some inconsistency there...

@fpistm As we discussed privately before, I've taken a stab at another class to control the SubGhz internal signals (NSS, Busy, Reset) and manage the radio interrupt. See matthijskooijman@1ca571f (single commit on top of this PR).

That class now also includes the SPI object for the radio that was still a global in your PR.

I'm not sure where this code should live exactly - it seems largely unrelated to SPI, so the SPI library is probably not the right place. So maybe in a library of its own, then?

@Oliv4945 I suspect the radio might be in reset by default, so you might need to clear the reset first. See the writeReset() method in the commit linked above for this.

It seems this interface is a bit confusing, though I'm not sure what would be better. From the user perspective, I guess there's basically two cases: Where the use controls the internal NSS signal manually (which works fine as case 1 now), and where the SPI library automatically controls it (which I guess feels more like case 3 than case 2, except that I think the timing of NSS switches is more like case 2 than case 3). Maybe the API to trigger automatic NSS control should be neither case 2 or 3, but be some SubGHZ_SPI-specific API?

In fact case 2 and 3 are the same as there is no hardware NSS. It is only to be consistent with SPI API. I do not want make dedicated SUBGHZSPI API. I see no good reason to diverge from default API.

I also wonder if it makes sense to have a SubGHZ_SPI constructor that accepts pins at all, though. Normally, the pins passed are passed through the pin maps to be checked, so you can only pass pins that are actually connected to the SPI block. However, for SubGHZ_SPI, this pin map is skipped (because of the spi_init exception), so any pins passed will work (in the sense that the SPI block controls the internal SPI signals, not the pins passed). Only when calling enableDebugPins() will the pins be applied to the pinmap (but any failures by incorrect pins are, I think, ignored). I guess this constructor could be useful when using enableDebugPins() to select to which pins the debug signals should be mapped exactly, except that they can only be mapped in exactly one place, so this does not add anything.

Right, it is only for enableDebugPins and aligned with standard API.

So maybe the SUBGHZSPIClass constructor with arguments can just be removed? This also makes (maybe) it a little more clear that case 3 (hardware control of NSS) is not supported (though I guess directly controlling the internal NSS signal might be more similar to case 3 than case 2, maybe...).

Well maybe. Don't we fallback to SPI constructor in that case ? Don't remember this.

Hi @matthijskooijman
I've updated my PR based on your comments/discussion.

Note: Default instance: SubGHZ_SPI removed

@fpistm I've updated this PR, with the following changes to the SPI library:

• I fixed spi_init check for SUBGHZSPI, it checked the wrong variable (handle->Instance wasn't set until after the check). This wasn't a problem before when SUBGHZSPIClass still passed actual pin numbers to SPIClass (which spi_init could resolve to the right SPI peripheral), but now that the pin numbers are replaced with NC this would trigger the "ERROR: at least one SPI pin has no peripheral" message. Fixing the check resolves this. I've also added a check that all pins must be NC for SUBGHZ and slightly refactored the code around there to reuse the handle->Init.NSS assignment code (which results in the same behavior since pin_ssel is NC).
• I added some using directives, since if you override a method in a subclass, other overloads (with different argument types) are not automatically inherited anymore. These directives make this inheritance explicit.
• Made the SPISettings constructors constexpr, so the class can be used on constexpr values.

I've put the first two in a fixup commit for easy review, this commit must be squashed before merging!

Additionally, I've added a commit that adds my SubGhz object, now in a separate library as discussed, adding a README, keywords.txt, library.properties and an example sketch.

I've changed the pin API to use bools instead of LOW/HIGH, I think that's the (marginally) better approach after all.

I've also added an spi_settings object to be used with beginTransaction() (since that method always requires a settings object to be passed). Using this also ensures the SPI speed is as fast as possible (up to 16Mhz is supported by the radio). It would be good if the speed was this high by default for the SubGHZ SPI object, but I'm not sure what the cleanest way for this is (we could call setClockDivider, but then we would have to know the input clock speed and back-calculate from Mhz to a divider, or we could preset the speed in spiSettings, but that is currently private).

I've updated our (private) RadioLib prototype branch to use this new SubGhz library, that works well for me now. Next I'll clean up that branch and produce a proper public PR from that.

Thanks @matthijskooijman

I've pushed a change to pio build to avoid issue raised by the ci. (required to rebase on top of main)
Still some typo to fix and applying astyle.

I agree for the fixup you can merge it. Pay attention to astyle for this commit.

I've pushed again to fix astyle and codespell.

Continuous integration still fails, since this now tries to compile the SubGhz library example for non-wl boards, which fails. I guess it would be useful to include this example in the CI build, but I'm not entirely sure what the best approach is (we could run platformio-builder.py twice here, once ignoring the SubGhz library and once for the STM32WL board, but maybe that makes the generated output a bit more confusing (since there will be a summary of build results half way then, I think. I haven't looked at the core_build workflow at all yet).

Seems our comments crossed. Ignoring the SubGhz library seems ok, maybe it can be added to the test build later. :-)

I've rebased on main and merged the fixup (it was 490afe0 if you want to review it again).

It can be ignored now. Even if it is not existing it should not be an issue. Thanks for the quick update.

@Oliv4945 Thanks for your review and suggestions, I've processed them in a new version.

@fpistm I'm done with this PR, so it can be merged as far as I'm concerned.

Thanks @matthijskooijman

LGTM even if PIO is CI is broken.