Add a function to activate CW mode #16

Miceuz · 2023-03-15T09:38:52Z

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

matthijskooijman

Thanks!

Overall, this PR looks good. Some remarks/questions:

You've also included a commit for RAK support, that probably warrants its own PR. Could you remove it (just force push to your branch with the commit removed).

Are you sure the power is directly specified in dB? The regular TX power is specified by index, with a non-trivial conversion from dB:

STM32LoRaWAN/src/STM32LoRaWAN.cpp

Lines 218 to 223 in 46d7e91

    
           bool STM32LoRaWAN::powerdB(int8_t db){ 
        
             // This uses knowledge about the radio implementation to calculate the 
        
             // index to use. See RegionCommonComputeTxPower() 
        
             int8_t index = -(db - 1) / 2; 
        
             return mibSetInt8("power", MIB_CHANNELS_TX_POWER, index); 
        
           }

Is there a special "no timeout" value? If so, the comments should specify it.

Miceuz · 2023-03-15T10:09:49Z

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

You are right about power units also.

Miceuz · 2023-03-15T10:15:07Z

Fixed that

matthijskooijman · 2023-03-15T12:10:51Z

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

Miceuz · 2023-03-15T14:19:23Z

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 );

fpistm

Small typos else LGTM.
Thanks for your contribution.

src/STM32LoRaWAN.h

matthijskooijman · 2023-03-23T10:19:13Z

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 · 2023-03-23T14:23:04Z

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

Fix a typo Co-authored-by: Frederic Pillon <frederic.pillon@st.com> Signed-off-by: Albertas Mickėnas <albertas@technarium.lt>

Miceuz · 2023-03-23T17:08:16Z

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 · 2023-03-23T17:55:29Z

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?

…_SetRfTxPower behaviour

… feature/CW-mode

fpistm · 2023-03-24T08:41:34Z

Looking at the code, when LoRaMacMlmeRequest is called, it calls:

STM32LoRaWAN/src/STM32CubeWL/LoRaWAN/Mac/LoRaMac.c

Line 6247 in d3aa385

    
           status = SetTxContinuousWave1( mlmeRequest->Req.TxCw.Timeout, mlmeRequest->Req.TxCw.Frequency, mlmeRequest->Req.TxCw.Power );

then SetTxContinuousWave:

STM32LoRaWAN/src/STM32CubeWL/LoRaWAN/Mac/LoRaMac.c

Line 4093 in d3aa385

Radio.SetTxContinuousWave( frequency, power, timeout );

with SetTxContinuousWave :

STM32LoRaWAN/src/STM32CubeWL/SubGHz_Phy/radio.h

Lines 297 to 304 in d3aa385

    
               /*! 
        
                * \brief Sets the radio in continuous wave transmission mode 
        
                * 
        
                * \param [in] freq       Channel RF frequency 
        
                * \param [in] power      Sets the output power [dBm] 
        
                * \param [in] time       Transmission mode timeout [s] 
        
                */ 
        
               void    ( *SetTxContinuousWave )( uint32_t freq, int8_t power, uint16_t time );

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 · 2023-03-24T08:52:51Z

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 · 2023-03-24T09:06:09Z

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 · 2023-03-24T10:07:51Z

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

LGTM. Thanks for the update.
Waiting @matthijskooijman feedback 😉

fpistm · 2023-04-11T08:15:48Z

Thanks @Miceuz to the issue submitted here: STMicroelectronics/STM32CubeWL#64

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

matthijskooijman

I was a bit confused by all the different code snippets, but finally found the time to look a bit closer. Looks good to me now, yes.

As for SUBGRF_SetRfTxPower(), AFAICS the problem is not actually the code, just the documentation problem, right? Reading your comments and the code right, the function is called with dBm, passes dBm on to SUBGRF_SetTxParams() and the >15 check makes sense, since LP goes up to 15dBm and HP up to 22dBm, so it should work as expected, right?

For reference, here is the upstream issue @Miceuz reported: STMicroelectronics/STM32CubeWL#64

fpistm · 2023-05-03T07:43:05Z

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

Miceuz mentioned this pull request Mar 15, 2023

CW mode support? #15

Closed

matthijskooijman requested changes Mar 15, 2023

View reviewed changes

Miceuz force-pushed the feature/CW-mode branch from 46d7e91 to 53c58f0 Compare March 15, 2023 10:13

Add a function to enter the continuous wave mode. Fix stm32duino#15

d8a7565

Miceuz force-pushed the feature/CW-mode branch from 53c58f0 to d8a7565 Compare March 15, 2023 10:14

Miceuz requested a review from matthijskooijman March 15, 2023 12:02

fpistm added the enhancement New feature or request label Mar 23, 2023

fpistm requested changes Mar 23, 2023

View reviewed changes

src/STM32LoRaWAN.h Outdated Show resolved Hide resolved

src/STM32LoRaWAN.h Outdated Show resolved Hide resolved

fpistm added this to the 0.1.1/0.2.0 milestone Mar 23, 2023

Miceuz and others added 2 commits March 23, 2023 19:05

Update src/STM32LoRaWAN.h

40f69bf

Fix a typo Co-authored-by: Frederic Pillon <frederic.pillon@st.com> Signed-off-by: Albertas Mickėnas <albertas@technarium.lt>

Update src/STM32LoRaWAN.h

2d79dc9

Fix a typo Co-authored-by: Frederic Pillon <frederic.pillon@st.com> Signed-off-by: Albertas Mickėnas <albertas@technarium.lt>

Miceuz added 2 commits March 23, 2023 19:59

Power is expected in dBm at the lower level except for strange SUBGRF…

770f1aa

…_SetRfTxPower behaviour

Merge branch 'feature/CW-mode' of github.com:Miceuz/STM32LoRaWAN into…

8005de9

… feature/CW-mode

fpistm reviewed Mar 24, 2023

View reviewed changes

fpistm approved these changes Apr 7, 2023

View reviewed changes

fpistm modified the milestones: 0.1.1, 0.1.2/0.2.0 Apr 11, 2023

matthijskooijman approved these changes Apr 30, 2023

View reviewed changes

fpistm merged commit d7dbe30 into stm32duino:main May 3, 2023

fpistm linked an issue May 3, 2023 that may be closed by this pull request

CW mode support? #15

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a function to activate CW mode #16

Add a function to activate CW mode #16

Miceuz commented Mar 15, 2023

matthijskooijman left a comment

Miceuz commented Mar 15, 2023

Miceuz commented Mar 15, 2023

matthijskooijman commented Mar 15, 2023

Miceuz commented Mar 15, 2023

fpistm left a comment

matthijskooijman commented Mar 23, 2023

fpistm commented Mar 23, 2023

Miceuz commented Mar 23, 2023

Miceuz commented Mar 23, 2023

fpistm commented Mar 24, 2023

Miceuz commented Mar 24, 2023

fpistm commented Mar 24, 2023

Miceuz commented Mar 24, 2023

fpistm left a comment

fpistm commented Apr 11, 2023

matthijskooijman left a comment

fpistm commented May 3, 2023 •

edited

Loading

	bool STM32LoRaWAN::powerdB(int8_t db){
	// This uses knowledge about the radio implementation to calculate the
	// index to use. See RegionCommonComputeTxPower()
	int8_t index = -(db - 1) / 2;
	return mibSetInt8("power", MIB_CHANNELS_TX_POWER, index);
	}

Add a function to activate CW mode #16

Add a function to activate CW mode #16

Conversation

Miceuz commented Mar 15, 2023

matthijskooijman left a comment

Choose a reason for hiding this comment

Miceuz commented Mar 15, 2023

Miceuz commented Mar 15, 2023

matthijskooijman commented Mar 15, 2023

Miceuz commented Mar 15, 2023

fpistm left a comment

Choose a reason for hiding this comment

matthijskooijman commented Mar 23, 2023

fpistm commented Mar 23, 2023

Miceuz commented Mar 23, 2023

Miceuz commented Mar 23, 2023

fpistm commented Mar 24, 2023

Miceuz commented Mar 24, 2023

fpistm commented Mar 24, 2023

Miceuz commented Mar 24, 2023

fpistm left a comment

Choose a reason for hiding this comment

fpistm commented Apr 11, 2023

matthijskooijman left a comment

Choose a reason for hiding this comment

fpistm commented May 3, 2023 • edited Loading

fpistm commented May 3, 2023 •

edited

Loading