Add a function to activate CW mode

[Miceuz]

It's just a wrapper function for underlying Semtech infrastructure for activating CW mode.

[Miceuz]

Argh, you are right, I have based my changes on previous branch. Will fix that.

You are right about power units also.

[Miceuz]

Fixed that

[matthijskooijman]

Seems you dropped the RAK commit, but haven't addressed the other questions about power and timeout yet?

[Miceuz]

Seems you dropped the RAK commit, but haven't addressed the other questions about power and timeout yet?

Yes, I have removed dBm mention from the documentation and no, there is no special "no timeout" value it seems:

UTIL_TIMER_Status_t UTIL_TIMER_SetPeriod(UTIL_TIMER_Object_t *TimerObject, uint32_t NewPeriodValue)
{
  UTIL_TIMER_Status_t  ret = UTIL_TIMER_OK;

  if(NULL == TimerObject)
  {
      ret = UTIL_TIMER_INVALID_PARAM;
  }
  else
  {
    TimerObject->ReloadValue = UTIL_TimerDriver.ms2Tick(NewPeriodValue);
    if(TimerExists(TimerObject))
    {
      (void)UTIL_TIMER_Stop(TimerObject);
      ret = UTIL_TIMER_Start(TimerObject);
    }
  }
  return ret;
}

Time is converted from seconds to ms in radio.c:

    uint32_t timeout = ( uint32_t )time * 1000;
    uint8_t antswitchpow;

    SUBGRF_SetRfFrequency( freq );

    antswitchpow = SUBGRF_SetRfTxPower( power );

    /* WORKAROUND - Trimming the output voltage power_ldo to 3.3V */
    SUBGRF_WriteRegister(REG_DRV_CTRL, 0x7 << 1);

    /* Set RF switch */
    SUBGRF_SetSwitch( antswitchpow, RFSWITCH_TX );

    SUBGRF_SetTxContinuousWave( );

    TimerSetValue( &TxTimeoutTimer, timeout );
    TimerStart( &TxTimeoutTimer );

[matthijskooijman]

Yes, I have removed dBm mention from the documentation and no, there is no special "no timeout" value it seems:

I wonder if the API should accept a power in dBm, just like the regular power setting function. Of course it is a testing function, so having a somewhat less friendly API that has a potentially hardware-dependent power argument might be acceptable (@fpistm, what do you think?), but in that case at least the documentation should say something about what the power values mean (even if it is just a reference to the relevant register in the STM32WL55 datasheet).

[fpistm]

Yes, I have removed dBm mention from the documentation and no, there is no special "no timeout" value it seems:

I wonder if the API should accept a power in dBm, just like the regular power setting function. Of course it is a testing function, so having a somewhat less friendly API that has a potentially hardware-dependent power argument might be acceptable (@fpistm, what do you think?), but in that case at least the documentation should say something about what the power values mean (even if it is just a reference to the relevant register in the STM32WL55 datasheet).

Agreed. * \param power TX power level [0-15]. is confusing as it seems it should be in dBm. So I would rewrite this: * \param powerdBm transmit power in dBm. and rename arg powerdBm

[Miceuz]

Yes, I have removed dBm mention from the documentation and no, there is no special "no timeout" value it seems:

I wonder if the API should accept a power in dBm, just like the regular power setting function. Of course it is a testing function, so having a somewhat less friendly API that has a potentially hardware-dependent power argument might be acceptable (@fpistm, what do you think?), but in that case at least the documentation should say something about what the power values mean (even if it is just a reference to the relevant register in the STM32WL55 datasheet).

Agreed. * \param power TX power level [0-15]. is confusing as it seems it should be in dBm. So I would rewrite this: * \param powerdBm transmit power in dBm. and rename arg powerdBm

Cool, I will re-calculate to index from dB in the same manner you have posted above int8_t index = -(db - 1) / 2;

[Miceuz]

I found something fishy in Semtech code. SUBGRF_SetRfTxPower declares as if it expects an index, not dBm, that's what initially threw me off:

uint8_t SUBGRF_SetRfTxPower(int8_t power)
Set the Tx End Device conducted power

Parameters:
[in] – power Tx power level [0..15]

Return values:
paSelect [RFO_LP, RFO_HP]

Then it does checks for PA selection:

    switch (TxConfig)
    {
        case RBI_CONF_RFO_LP_HP:
        {
            if (power > 15)
            {
                paSelect = RFO_HP;
            }
            else
            {
                paSelect = RFO_LP;
            }
            break;
        }

And then calls SUBGRF_SetTxParams which expects dBm:

void SUBGRF_SetTxParams(uint8_t paSelect, int8_t power, RadioRampTimes_t rampTime)
Sets the transmission parameters

Parameters:
[in] – paSelect RegPaConfig PaSelect value (RFO_LP, RFO_HP, etc)
[in] – power RF output power [-18..13] dBm
[in] – rampTime Transmission ramp up time

Thus the check for 15 in SUBGRF_SetRfTxPower makes no sense and also [in] – power Tx power level [0..15] is wrong as SUBGRF_SetTxParams expects [-18..13] dBm.

SUBGRF_SetRfTxPower is called also in RadioSetTxConfig that declares it expects power in dBm, and that is called by RegionEU868TxConfig where power index is actually converted to dBm by RegionCommonComputeTxPower.

So yeah, I think SUBGRF_SetRfTxPower needs to be fixed.

Can anyone guide me on >15 check?

[fpistm]

Looking at the code, when LoRaMacMlmeRequest is called, it calls: https://github.com/stm32duino/STM32LoRaWAN/blob/d3aa3852d36afe0ad4e46d0a4524cc13957175fc/src/STM32CubeWL/LoRaWAN/Mac/LoRaMac.c#L6247 then SetTxContinuousWave: https://github.com/stm32duino/STM32LoRaWAN/blob/d3aa3852d36afe0ad4e46d0a4524cc13957175fc/src/STM32CubeWL/LoRaWAN/Mac/LoRaMac.c#L4093 with SetTxContinuousWave: https://github.com/stm32duino/STM32LoRaWAN/blob/d3aa3852d36afe0ad4e46d0a4524cc13957175fc/src/STM32CubeWL/SubGHz_Phy/radio.h#L297-L304

So I think we could only deal with dBm while it is explicitly describe, as this feature is for test purpose, I think it is acceptable.

@matthijskooijman ?

[Miceuz]

Yes, and then RadioSetTxContinuousWave calls SUBGRF_SetRfTxPower that expects Tx power level [0..15] that calls SUBGRF_SetTxParams that expects RF output power [-18..13] dBm.

So it's clearly dBm, but I think there's a bug in SUBGRF_SetRfTxPower - it will never select RFO_HP

[fpistm]

Yes, and then RadioSetTxContinuousWave calls SUBGRF_SetRfTxPower that expects Tx power level [0..15] that calls SUBGRF_SetTxParams that expects RF output power [-18..13] dBm.

So it's clearly dBm, but I think there's a bug in SUBGRF_SetRfTxPower - it will never select RFO_HP

Right, I guess an issue should be filled in https://github.com/STMicroelectronics/STM32CubeWL/issues

[Miceuz]

Yes, and then RadioSetTxContinuousWave calls SUBGRF_SetRfTxPower that expects Tx power level [0..15] that calls SUBGRF_SetTxParams that expects RF output power [-18..13] dBm. So it's clearly dBm, but I think there's a bug in SUBGRF_SetRfTxPower - it will never select RFO_HP

Right, I guess an issue should be filled in https://github.com/STMicroelectronics/STM32CubeWL/issues

Ok, will do. So this PR then can be merged - I have altered the documentation strings to represent dBm.

[fpistm]

Thanks @Miceuz to the issue submitted here: https://github.com/STMicroelectronics/STM32CubeWL/issues/64

@matthijskooijman are you agreed with this PR now? I guess it could be merged safely as it does not impact other API.

[fpistm]

Agreed, documentations are not coherent. Let's see how it will be handled on STM32CubeWL repo. Anyway, this PR can be merged safely.