ARMmbed / mbed-semtech-lora-rf-drivers

Semtech's LoRa RF drivers for mbed OS
Other
31 stars 25 forks source link

Setting the SX1276 Variant Type #27

Closed mattbrown015 closed 6 years ago

mattbrown015 commented 6 years ago

Description

SX1276_LoRaRadio::set_sx1276_variant_type uses ant_switch to set radio_variant.

None of this means very much to me but I do know it results in the wrong thing happening on our board. I think there needs to be some more thought and flexibility put into this but unfortunately I don't understand what configurations are possible and/or likely.

Our board connects SX1276 RXTX/RF_mod to the RF switch CTRL. I think this matches what it says here SX1276MB1xAS:

The status of the RxTx pin is also used at boot up to detect the board connected. Reading a ’1’ means the board connected is a SX1276MB1LAS and reading a ‘0’ indicates a SX1276MB1MAS. For production, the RxTx pin can be directly driven from the RF chipset by removing R16 and fitting R15.

Consequently in the software _rf_ctrls.ant_switch != NC and radio_variant == SX1276UNDEFINED.

The real problem is in SX1276_LoRaRadio::get_pa_conf_reg because it assumes that if radio_variant == SX1276UNDEFINED then the high-stability PA is being used:

uint8_t SX1276_LoRaRadio::get_pa_conf_reg(uint32_t channel)
{
    if (radio_variant == SX1276UNDEFINED) {
        return RF_PACONFIG_PASELECT_PABOOST;
    } else if (channel > RF_MID_BAND_THRESH) {
        if (radio_variant == SX1276MB1LAS) {
            return RF_PACONFIG_PASELECT_PABOOST;
        } else {
            return RF_PACONFIG_PASELECT_RFO;
        }
    } else {
        return RF_PACONFIG_PASELECT_RFO;
    }
}

To make our board work I made SX1276UNDEFINED select the high efficiency PAs.

Is using the high-stability PA really the most typical configuration? I think this is what this is saying.

One quick solution to add flexibility would be to add a default PA_SELECT library configuration item.

Issue request type

[ ] Question [X] Enhancement [ ] Bug

ciarmcom commented 6 years ago

ARM Internal Ref: IOTCELL-1177

mattbrown015 commented 6 years ago

@hasnainvirk I was hoping you might have some thoughts about this. :-)

I got my software working by forking and adding a configuration item, as I suggested above. https://github.com/mattbrown015/mbed-semtech-lora-rf-drivers

While mbed-semtech-lora-rf-drivers isn't changing nearly as much as mbed-os I would still like to change back to the upstream.

What do you think? Would you accept a PR? Can you think of a better way to add the flexibility?

@hasnainvirk Perhaps it's not your call??!!

kivaisan commented 6 years ago

@mattbrown015 Please make a PR and we can review it.

hasnainvirk commented 6 years ago

@mattbrown015 Yeah, please go ahead and make a PR and we can review it there.

hasnainvirk commented 6 years ago

@mattbrown015 I gather from the description that you have ant_switch connected and it's driven high. So you are ending up with SX1276MB1LAS variation. In that case, you will not fall in else branch in set_sx1276_varient_type( ). My suggestion is that we do something like this:

In mbed_lib.json

 "force-paboost": {
            "help": "Used for enforcing high efficiency power amplifier regardless of chip variant. [Warning]: Check board circuitry if PA boost can be enabled or not.",
            "value": false
        }

For boards like yours, you could set it to true.

In get_pa_conf_reg( ):

#if MBED_CONF_SX1276_LORA_DRIVER_FORCE_PABOOST
      return RF_PACONFIG_PASELECT_PABOOST;
#else
     if (radio_variant == SX1272UNDEFINED) {
        return RF_PACONFIG_PASELECT_PABOOST;
    } else if (radio_variant == SX1272MB1DCS) {
        return RF_PACONFIG_PASELECT_PABOOST;
    } else {
        return RF_PACONFIG_PASELECT_RFO;
    }
#endif

@kivaisan What do you think ?

kivaisan commented 6 years ago

@hasnainvirk I think your proposal does not work with 1276 high/low band selection.

hasnainvirk commented 6 years ago
#if MBED_CONF_SX1276_LORA_DRIVER_FORCE_PABOOST
      if (channel > RF_MID_BAND_THRESH) {
                return RF_PACONFIG_PASELECT_PABOOST;
       } else {
                return RF_PACONFIG_PASELECT_RFO;
        }
#else
    if (radio_variant == SX1276UNDEFINED) {
        return RF_PACONFIG_PASELECT_PABOOST;
    } else if (channel > RF_MID_BAND_THRESH) {
        if (radio_variant == SX1276MB1LAS) {
            return RF_PACONFIG_PASELECT_PABOOST;
        } else {
            return RF_PACONFIG_PASELECT_RFO;
        }
    } else {
        return RF_PACONFIG_PASELECT_RFO;
    }
#endif

@kivaisan how does that look for SX1276 ?

kivaisan commented 6 years ago

@hasnainvirk maybe that would work until somebody again has a different hw set up.

mattbrown015 commented 6 years ago

I gather from the description that you have ant_switch connected and it's driven high.

@hasnainvirk Not quite. I've got ant_switch set to NC which results in SX1272UNDEFINED which is not the same as SX1276MB1LAS.

As far as I can tell:

(Now I think about it in more detail.) What I've done just allows the selection of the variant when ant_switch = NC. Nothing changes if ant_switch is being used hence anyone actually using a SX1272MB2xAS board is unaffected. So I don't think it can do any harm!

My solution maintains some code, ant_switch, set_sx1276_variant_type and get_pa_conf_reg, which is pointless when the configuration has been decided and fixed. But, I'm not worried about optimising code at the moment, although people with smaller targets might be, and sorting it out at compile-time would be a much bigger change.

I'll create the PR.

kivaisan commented 6 years ago

I merged your PR, so I think this issue can be closed?

mattbrown015 commented 6 years ago

I think so too! Thx :-)