Skip to content

Force LDO option set or unset #668

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 29, 2023
Merged

Conversation

zbx-sadman
Copy link
Contributor

Some remote clients needs LDO using on any Spreading Factor. CDEbyte E32 (and similar) modules for example.

Some remote clients needs LDO using on any Spreading Factor. CDEbyte E32 for example.
Copy link
Owner

@sandeepmistry sandeepmistry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @zbx-sadman,

Thanks for taking the time to open this pull request, what do think about splitting the API to enableLowDataRateOptimize() and disableLowDataRateOptimize()?

@zbx-sadman
Copy link
Contributor Author

Hello @sandeepmistry

Library author known better how to make his code pretty and readable, i guess.

Honestly, i modified source a bit deeper and it can be written now as follows:

LoRa.h

class LoRaClass : public Stream {
public:
  LoRaClass();
...
  void setLdoFlagForced(const boolean);  // can be replaced with enableLowDataRateOptimize() & disableLowDataRateOptimize()
  void enableLowDataRateOptimize();
  void disableLowDataRateOptimize();
...
private:
...
  boolean _ldoForced;
...
};

LoRa.cpp

void LoRaClass::setLdoFlag()
{
  boolean ldoOn = true;

  if (!_ldoForced) {
     // Section 4.1.1.5
     long symbolDuration = 1000 / ( getSignalBandwidth() / (1L << getSpreadingFactor()) ) ;

     // Section 4.1.1.6
     ldoOn = symbolDuration > 16;
  }

  uint8_t config3 = readRegister(REG_MODEM_CONFIG_3);
  bitWrite(config3, 3, ldoOn);
  writeRegister(REG_MODEM_CONFIG_3, config3);
}

// can be replaced with enableLowDataRateOptimize() & disableLowDataRateOptimize()
void LoRaClass::setLdoFlagForced(const boolean ldoForced)
{
  _ldoForced = ldoForced; 
  setLdoFlag();
}

void LoRaClass::enableLowDataRateOptimize() 
{
  _ldoForced = true; 
  setLdoFlag();
}

void LoRaClass::disableLowDataRateOptimize() 
{
  _ldoForced = false; 
  setLdoFlag();
}

Let me know if i must create another pull-request.

Thanks for your helpful library.

@sandeepmistry
Copy link
Owner

Hi @zbx-sadman,

Thanks for considering the feedback, I would suggest making the void setLdoFlagForced(const boolean); private, and removing _ldoForced. Other than your comments in #668 (comment) look good. Feel free to make edits to this pull request or make a new one.

@zbx-sadman
Copy link
Contributor Author

Hello @sandeepmistry, i've add separated setter methods for LDO flag.
This is right way for library?

@sandeepmistry
Copy link
Owner

@zbx-sadman yes, that looks good. I've pushed some minor changes in 1928aab to update the keywords.txt and move the private function declaration in the .h earlier in the file.

@sandeepmistry sandeepmistry merged commit 7c2ebfd into sandeepmistry:master Oct 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants