From da6072cf7324de205012de8b407c23bc27b0321a Mon Sep 17 00:00:00 2001
From: BarryPSmith <smith.barryp@gmail.com>
Date: Fri, 8 Nov 2019 08:20:52 -0800
Subject: [PATCH 1/5] Adjusted setPaConfig in SX126x setOutputPower.

---
 src/modules/SX1261.cpp | 40 ++++++++++++++++++++++++++++++++++++++++
 src/modules/SX1261.h   | 15 ++++++++++++++-
 src/modules/SX1262.cpp | 27 +++++++++++++++++++++------
 3 files changed, 75 insertions(+), 7 deletions(-)
 create mode 100644 src/modules/SX1261.cpp

diff --git a/src/modules/SX1261.cpp b/src/modules/SX1261.cpp
new file mode 100644
index 00000000..15d6e1fa
--- /dev/null
+++ b/src/modules/SX1261.cpp
@@ -0,0 +1,40 @@
+#include "SX1261.h"
+
+// note that this is untested (lacking the hardware) and based purely on the datasheet.
+int16_t SX1261::setOutputPower(int8_t power) {  
+  // check allowed power range
+  if (!((power >= -17) && (power <= 14))) {
+    return(ERR_INVALID_OUTPUT_POWER);
+  }
+
+  // get current OCP configuration
+  uint8_t ocp = 0;
+  int16_t state = readRegister(SX126X_REG_OCP_CONFIGURATION, &ocp, 1);
+  if (state != ERR_NONE) {
+    return(state);
+  }
+
+  // set PA config for optimal consumption as described in section 13-21 of the datasheet:
+  // the final column of Table 13-21 suggests that the value passed in SetTxParams is actually scaled depending on the parameters of setPaConfig. However, testing suggests this isn't the case.
+  if (power > 10) {
+    state = SX126x::setPaConfig(0x04, 0x00, SX126X_PA_CONFIG_HP_MAX/*0x07*/);
+  }
+  else {
+    state = SX126x::setPaConfig(0x01, 0x00, 0x05);
+  }
+  if (state != ERR_NONE) {
+    return(state);
+  }
+  // TODO investigate if better power efficiency can be achieved using undocumented even lower settings
+
+  // set output power
+  // TODO power ramp time configuration
+  state = SX126x::setTxParams(power);
+  if (state != ERR_NONE) {
+    return state;
+  }
+
+
+  // restore OCP configuration
+  return(writeRegister(SX126X_REG_OCP_CONFIGURATION, &ocp, 1));
+}
diff --git a/src/modules/SX1261.h b/src/modules/SX1261.h
index cb8a5758..b9c8c741 100644
--- a/src/modules/SX1261.h
+++ b/src/modules/SX1261.h
@@ -11,6 +11,19 @@
 #define SX126X_PA_CONFIG_SX1262                       0x00
 
 // TODO: implement SX1261 class
-using SX1261 = SX1262;
+class SX1261 : public SX1262 {
+public:
+  /*!
+    \brief Default constructor.
+
+    \param mod Instance of Module that will be used to communicate with the radio.
+  */
+  SX1261(Module* mod)
+    : SX1262(mod) {
+  }
+
+  int16_t setOutputPower(int8_t power);
+};
+
 
 #endif
diff --git a/src/modules/SX1262.cpp b/src/modules/SX1262.cpp
index 0bcce47d..d2482593 100644
--- a/src/modules/SX1262.cpp
+++ b/src/modules/SX1262.cpp
@@ -85,7 +85,7 @@ int16_t SX1262::setFrequency(float freq, bool calibrate) {
 
 int16_t SX1262::setOutputPower(int8_t power) {
   // check allowed power range
-  if(!((power >= -17) && (power <= 22))) {
+  if(!((power >= -9) && (power <= 22))) {
     return(ERR_INVALID_OUTPUT_POWER);
   }
 
@@ -96,16 +96,31 @@ int16_t SX1262::setOutputPower(int8_t power) {
     return(state);
   }
 
-  // enable high power PA for output power higher than 14 dBm
-  if(power > 13) {
-    SX126x::setPaConfig(0x04, SX126X_PA_CONFIG_SX1262);
+  // set PA config for optimal consumption as described in section 13-21 of the datasheet:
+  // the final column of Table 13-21 suggests that the value passed in SetTxParams is actually scaled depending on the parameters of setPaConfig. However, testing suggests this isn't the case.
+  if (power >= 21) {
+    state = SX126x::setPaConfig(0x04, SX126X_PA_CONFIG_SX1262, SX126X_PA_CONFIG_HP_MAX/*0x07*/);
+  } else if (power >= 18) {
+    state = SX126x::setPaConfig(0x03, SX126X_PA_CONFIG_SX1262, 0x05);
+  } else if (power >= 15) {
+    state = SX126x::setPaConfig(0x02, SX126X_PA_CONFIG_SX1262, 0x03);
   } else {
-    SX126x::setPaConfig(0x04, SX126X_PA_CONFIG_SX1261);
+    state = SX126x::setPaConfig(0x02, SX126X_PA_CONFIG_SX1262, 0x02);
   }
+  if (state != ERR_NONE) {
+    return(state);
+  }
+  // TODO investigate if better power efficiency can be achieved using undocumented even lower settings
+
+  // note that we set SX126X_PA_CONFIG_SX1262 for all power levels - setting SX126X_PA_CONFIG_SX1261 causes no output (on the nameless module I have).
 
   // set output power
   // TODO power ramp time configuration
-  SX126x::setTxParams(power);
+  state = SX126x::setTxParams(power);
+  if (state != ERR_NONE) {
+    return state;
+  }
+
 
   // restore OCP configuration
   return(writeRegister(SX126X_REG_OCP_CONFIGURATION, &ocp, 1));

From 111d9fa6253a8bba6d086a11a588bf8b06ae03a2 Mon Sep 17 00:00:00 2001
From: BarryPSmith <smith.barryp@gmail.com>
Date: Fri, 8 Nov 2019 08:44:42 -0800
Subject: [PATCH 2/5] Missed SX1261 PA Config flag in previous commit

---
 src/modules/SX1261.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/modules/SX1261.cpp b/src/modules/SX1261.cpp
index 15d6e1fa..6f86e79b 100644
--- a/src/modules/SX1261.cpp
+++ b/src/modules/SX1261.cpp
@@ -17,10 +17,10 @@ int16_t SX1261::setOutputPower(int8_t power) {
   // set PA config for optimal consumption as described in section 13-21 of the datasheet:
   // the final column of Table 13-21 suggests that the value passed in SetTxParams is actually scaled depending on the parameters of setPaConfig. However, testing suggests this isn't the case.
   if (power > 10) {
-    state = SX126x::setPaConfig(0x04, 0x00, SX126X_PA_CONFIG_HP_MAX/*0x07*/);
+    state = SX126x::setPaConfig(0x04, SX126X_PA_CONFIG_SX1261, 0x00);
   }
   else {
-    state = SX126x::setPaConfig(0x01, 0x00, 0x05);
+    state = SX126x::setPaConfig(0x01, SX126X_PA_CONFIG_SX1261, 0x00);
   }
   if (state != ERR_NONE) {
     return(state);

From 5b04badb3628c1162fc8eab9bfe296bcf227d904 Mon Sep 17 00:00:00 2001
From: BarryPSmith <smith.barryp@gmail.com>
Date: Mon, 11 Nov 2019 11:26:41 -0800
Subject: [PATCH 3/5] Implemented power scaling in setOutputPower, in
 accordance with datasheet.

---
 src/modules/SX1262.cpp | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/src/modules/SX1262.cpp b/src/modules/SX1262.cpp
index d2482593..d4342243 100644
--- a/src/modules/SX1262.cpp
+++ b/src/modules/SX1262.cpp
@@ -85,27 +85,37 @@ int16_t SX1262::setFrequency(float freq, bool calibrate) {
 
 int16_t SX1262::setOutputPower(int8_t power) {
   // check allowed power range
-  if(!((power >= -9) && (power <= 22))) {
+  if (!((power >= -17) && (power <= 22))) {
     return(ERR_INVALID_OUTPUT_POWER);
   }
 
   // get current OCP configuration
   uint8_t ocp = 0;
   int16_t state = readRegister(SX126X_REG_OCP_CONFIGURATION, &ocp, 1);
-  if(state != ERR_NONE) {
+  if (state != ERR_NONE) {
     return(state);
   }
 
+  int8_t scaledPower;
   // set PA config for optimal consumption as described in section 13-21 of the datasheet:
-  // the final column of Table 13-21 suggests that the value passed in SetTxParams is actually scaled depending on the parameters of setPaConfig. However, testing suggests this isn't the case.
+  // the final column of Table 13-21 suggests that the value passed in SetTxParams 
+  // is actually scaled depending on the parameters of setPaConfig.
+  // Testing confirms this is approximately right
   if (power >= 21) {
     state = SX126x::setPaConfig(0x04, SX126X_PA_CONFIG_SX1262, SX126X_PA_CONFIG_HP_MAX/*0x07*/);
-  } else if (power >= 18) {
+    scaledPower = power;
+  }
+  else if (power >= 18) {
     state = SX126x::setPaConfig(0x03, SX126X_PA_CONFIG_SX1262, 0x05);
-  } else if (power >= 15) {
+    scaledPower = power + 2;
+  }
+  else if (power >= 15) {
     state = SX126x::setPaConfig(0x02, SX126X_PA_CONFIG_SX1262, 0x03);
-  } else {
+    scaledPower = power + 5;
+  }
+  else {
     state = SX126x::setPaConfig(0x02, SX126X_PA_CONFIG_SX1262, 0x02);
+    scaledPower = power + 8;
   }
   if (state != ERR_NONE) {
     return(state);
@@ -116,7 +126,7 @@ int16_t SX1262::setOutputPower(int8_t power) {
 
   // set output power
   // TODO power ramp time configuration
-  state = SX126x::setTxParams(power);
+  state = SX126x::setTxParams(scaledPower);
   if (state != ERR_NONE) {
     return state;
   }

From 621da8a11d305bbad317792560000d345119e7c0 Mon Sep 17 00:00:00 2001
From: BarryPSmith <smith.barryp@gmail.com>
Date: Tue, 12 Nov 2019 10:28:14 -0800
Subject: [PATCH 4/5] Implemented optimal PA settings for SX1268. Ensured OCP
 is always restored when changing power. Slight refactor to avoid duplicated
 SX1262 / SX1268 code.

---
 src/modules/SX1261.cpp | 42 +++++++++++++++++++++-----------------
 src/modules/SX1261.h   | 24 +++++++++++-----------
 src/modules/SX1262.cpp | 46 ++++++++++++------------------------------
 src/modules/SX1262.h   |  2 --
 src/modules/SX1268.cpp | 19 +++++++++++++----
 src/modules/SX126x.cpp | 26 ++++++++++++++++++++++++
 src/modules/SX126x.h   |  3 +++
 7 files changed, 93 insertions(+), 69 deletions(-)

diff --git a/src/modules/SX1261.cpp b/src/modules/SX1261.cpp
index 6f86e79b..8d8cd0ab 100644
--- a/src/modules/SX1261.cpp
+++ b/src/modules/SX1261.cpp
@@ -14,27 +14,33 @@ int16_t SX1261::setOutputPower(int8_t power) {
     return(state);
   }
 
-  // set PA config for optimal consumption as described in section 13-21 of the datasheet:
-  // the final column of Table 13-21 suggests that the value passed in SetTxParams is actually scaled depending on the parameters of setPaConfig. However, testing suggests this isn't the case.
-  if (power > 10) {
+  state = setOptimalLowPowerPaConfig(&power);
+
+  // set output power
+  // TODO power ramp time configuration
+  if (state == ERR_NONE) {
+    state = SX126x::setTxParams(power);
+  }
+
+  // restore OCP configuration
+  int16_t state2 = writeRegister(SX126X_REG_OCP_CONFIGURATION, &ocp, 1);
+
+  if (state != ERR_NONE) {
+    return state;
+  } else {
+    return state2;
+  }
+}
+
+int16_t SX1261::setOptimalLowPowerPaConfig(int8_t* inOutPower)
+{
+  int16_t state;
+  if (*inOutPower > 10) {
     state = SX126x::setPaConfig(0x04, SX126X_PA_CONFIG_SX1261, 0x00);
   }
   else {
     state = SX126x::setPaConfig(0x01, SX126X_PA_CONFIG_SX1261, 0x00);
+    *inOutPower -= 4;
   }
-  if (state != ERR_NONE) {
-    return(state);
-  }
-  // TODO investigate if better power efficiency can be achieved using undocumented even lower settings
-
-  // set output power
-  // TODO power ramp time configuration
-  state = SX126x::setTxParams(power);
-  if (state != ERR_NONE) {
-    return state;
-  }
-
-
-  // restore OCP configuration
-  return(writeRegister(SX126X_REG_OCP_CONFIGURATION, &ocp, 1));
+  return state;
 }
diff --git a/src/modules/SX1261.h b/src/modules/SX1261.h
index b9c8c741..4b1ce8fc 100644
--- a/src/modules/SX1261.h
+++ b/src/modules/SX1261.h
@@ -6,23 +6,23 @@
 #include "SX126x.h"
 #include "SX1262.h"
 
-//SX126X_CMD_SET_PA_CONFIG
-#define SX126X_PA_CONFIG_SX1261                       0x01
-#define SX126X_PA_CONFIG_SX1262                       0x00
 
 // TODO: implement SX1261 class
 class SX1261 : public SX1262 {
-public:
-  /*!
-    \brief Default constructor.
+  public:
+    /*!
+      \brief Default constructor.
 
-    \param mod Instance of Module that will be used to communicate with the radio.
-  */
-  SX1261(Module* mod)
-    : SX1262(mod) {
-  }
+      \param mod Instance of Module that will be used to communicate with the radio.
+    */
+    SX1261(Module* mod)
+      : SX1262(mod) {
+    }
 
-  int16_t setOutputPower(int8_t power);
+    int16_t setOutputPower(int8_t power);
+
+  private:
+    int16_t setOptimalLowPowerPaConfig(int8_t* inOutPower);
 };
 
 
diff --git a/src/modules/SX1262.cpp b/src/modules/SX1262.cpp
index d4342243..94f94837 100644
--- a/src/modules/SX1262.cpp
+++ b/src/modules/SX1262.cpp
@@ -85,7 +85,7 @@ int16_t SX1262::setFrequency(float freq, bool calibrate) {
 
 int16_t SX1262::setOutputPower(int8_t power) {
   // check allowed power range
-  if (!((power >= -17) && (power <= 22))) {
+  if (!(power >= -17 && power <= 22)) {
     return(ERR_INVALID_OUTPUT_POWER);
   }
 
@@ -96,42 +96,22 @@ int16_t SX1262::setOutputPower(int8_t power) {
     return(state);
   }
 
-  int8_t scaledPower;
-  // set PA config for optimal consumption as described in section 13-21 of the datasheet:
-  // the final column of Table 13-21 suggests that the value passed in SetTxParams 
-  // is actually scaled depending on the parameters of setPaConfig.
-  // Testing confirms this is approximately right
-  if (power >= 21) {
-    state = SX126x::setPaConfig(0x04, SX126X_PA_CONFIG_SX1262, SX126X_PA_CONFIG_HP_MAX/*0x07*/);
-    scaledPower = power;
-  }
-  else if (power >= 18) {
-    state = SX126x::setPaConfig(0x03, SX126X_PA_CONFIG_SX1262, 0x05);
-    scaledPower = power + 2;
-  }
-  else if (power >= 15) {
-    state = SX126x::setPaConfig(0x02, SX126X_PA_CONFIG_SX1262, 0x03);
-    scaledPower = power + 5;
-  }
-  else {
-    state = SX126x::setPaConfig(0x02, SX126X_PA_CONFIG_SX1262, 0x02);
-    scaledPower = power + 8;
-  }
-  if (state != ERR_NONE) {
-    return(state);
-  }
-  // TODO investigate if better power efficiency can be achieved using undocumented even lower settings
-
-  // note that we set SX126X_PA_CONFIG_SX1262 for all power levels - setting SX126X_PA_CONFIG_SX1261 causes no output (on the nameless module I have).
+  // this function sets the optimal PA settings 
+  // and scales our requested power based
+  state = SX126x::setOptimalHiPowerPaConfig(&power);
 
   // set output power
   // TODO power ramp time configuration
-  state = SX126x::setTxParams(scaledPower);
-  if (state != ERR_NONE) {
-    return state;
+  if (state == ERR_NONE) {
+    state = SX126x::setTxParams(power);
   }
 
-
   // restore OCP configuration
-  return(writeRegister(SX126X_REG_OCP_CONFIGURATION, &ocp, 1));
+  int16_t state2 = writeRegister(SX126X_REG_OCP_CONFIGURATION, &ocp, 1);
+  
+  if (state != ERR_NONE) {
+    return state;
+  } else {
+    return state2;
+  }
 }
diff --git a/src/modules/SX1262.h b/src/modules/SX1262.h
index 88a49576..b9a26a5f 100644
--- a/src/modules/SX1262.h
+++ b/src/modules/SX1262.h
@@ -6,8 +6,6 @@
 #include "SX126x.h"
 
 //SX126X_CMD_SET_PA_CONFIG
-#define SX126X_PA_CONFIG_SX1261                       0x01
-#define SX126X_PA_CONFIG_SX1262                       0x00
 
 /*!
   \class SX1262
diff --git a/src/modules/SX1268.cpp b/src/modules/SX1268.cpp
index c0b01a80..a752c638 100644
--- a/src/modules/SX1268.cpp
+++ b/src/modules/SX1268.cpp
@@ -78,6 +78,9 @@ int16_t SX1268::setFrequency(float freq, bool calibrate) {
 
 int16_t SX1268::setOutputPower(int8_t power) {
   // check allowed power range
+  // TODO with optimal PA config
+  // it's likely possible this could be expanded to go down to -17 just like for SX1262.
+  // but the datasheet doesn't explicitly state that and I don't have SX1268 unit to test with
   if(!((power >= -9) && (power <= 22))) {
     return(ERR_INVALID_OUTPUT_POWER);
   }
@@ -89,13 +92,21 @@ int16_t SX1268::setOutputPower(int8_t power) {
     return(state);
   }
 
-  // enable high power PA
-  SX126x::setPaConfig(0x04, SX126X_PA_CONFIG_SX1268);
+  // enable optimal PA
+  state = SX126x::setOptimalHiPowerPaConfig(&power);
 
   // set output power
   // TODO power ramp time configuration
-  SX126x::setTxParams(power);
+  if (state == ERR_NONE) {
+    state = SX126x::setTxParams(power);
+  }
 
   // restore OCP configuration
-  return(writeRegister(SX126X_REG_OCP_CONFIGURATION, &ocp, 1));
+  int16_t state2 = writeRegister(SX126X_REG_OCP_CONFIGURATION, &ocp, 1);
+
+  if (state != ERR_NONE) {
+    return state;
+  } else {
+    return state2;
+  }
 }
diff --git a/src/modules/SX126x.cpp b/src/modules/SX126x.cpp
index e47ad31c..2e759226 100644
--- a/src/modules/SX126x.cpp
+++ b/src/modules/SX126x.cpp
@@ -1127,6 +1127,32 @@ int16_t SX126x::setTxParams(uint8_t power, uint8_t rampTime) {
   return(SPIwriteCommand(SX126X_CMD_SET_TX_PARAMS, data, 2));
 }
 
+// set PA config for optimal consumption as described in section 13-21 of the datasheet:
+// the final column of Table 13-21 suggests that the value passed in SetTxParams 
+// is actually scaled depending on the parameters of setPaConfig.
+// Testing confirms this is approximately right
+int16_t SX126x::setOptimalHiPowerPaConfig(int8_t* inOutPower)
+{
+  int16_t state;
+  if (*inOutPower >= 21) {
+    state = SX126x::setPaConfig(0x04, SX126X_PA_CONFIG_SX1262_8, SX126X_PA_CONFIG_HP_MAX/*0x07*/);
+  }
+  else if (*inOutPower >= 18) {
+    state = SX126x::setPaConfig(0x03, SX126X_PA_CONFIG_SX1262_8, 0x05);
+    *inOutPower += 2;
+  }
+  else if (*inOutPower >= 15) {
+    state = SX126x::setPaConfig(0x02, SX126X_PA_CONFIG_SX1262_8, 0x03);
+    *inOutPower += 5;
+  }
+  else {
+    state = SX126x::setPaConfig(0x02, SX126X_PA_CONFIG_SX1262_8, 0x02);
+    *inOutPower += 8;
+  }
+  // TODO investigate if better power efficiency can be achieved using undocumented even lower settings
+  return state;
+}
+
 int16_t SX126x::setModulationParams(uint8_t sf, uint8_t bw, uint8_t cr, uint8_t ldro) {
   // calculate symbol length and enable low data rate optimization, if needed
   if(ldro == 0xFF) {
diff --git a/src/modules/SX126x.h b/src/modules/SX126x.h
index 641d6445..2bcd47ff 100644
--- a/src/modules/SX126x.h
+++ b/src/modules/SX126x.h
@@ -152,6 +152,8 @@
 //SX126X_CMD_SET_PA_CONFIG
 #define SX126X_PA_CONFIG_HP_MAX                       0x07
 #define SX126X_PA_CONFIG_PA_LUT                       0x01
+#define SX126X_PA_CONFIG_SX1261                       0x01
+#define SX126X_PA_CONFIG_SX1262_8                     0x00
 
 //SX126X_CMD_SET_RX_TX_FALLBACK_MODE
 #define SX126X_RX_TX_FALLBACK_MODE_FS                 0x40        //  7     0     after Rx/Tx go to: FS mode
@@ -739,6 +741,7 @@ class SX126x: public PhysicalLayer {
     int16_t calibrateImage(uint8_t* data);
     uint8_t getPacketType();
     int16_t setTxParams(uint8_t power, uint8_t rampTime = SX126X_PA_RAMP_200U);
+    int16_t setOptimalHiPowerPaConfig(int8_t* inOutPower);
     int16_t setModulationParams(uint8_t sf, uint8_t bw, uint8_t cr, uint8_t ldro = 0xFF);
     int16_t setModulationParamsFSK(uint32_t br, uint8_t pulseShape, uint8_t rxBw, uint32_t freqDev);
     int16_t setPacketParams(uint16_t preambleLength, uint8_t crcType, uint8_t payloadLength = 0xFF, uint8_t headerType = SX126X_LORA_HEADER_EXPLICIT, uint8_t invertIQ = SX126X_LORA_IQ_STANDARD);

From fb872dd41306a8870b8522a35fea68935cab0c66 Mon Sep 17 00:00:00 2001
From: BarryPSmith <smith.barryp@gmail.com>
Date: Wed, 13 Nov 2019 05:02:22 -0800
Subject: [PATCH 5/5] Comment and error checking cleanup. Fixed power
 adjustment for SX1261

---
 src/modules/SX1261.cpp | 26 +++++++++++++++-----------
 src/modules/SX1261.h   | 13 ++++++++++---
 src/modules/SX1262.cpp | 21 ++++++++++-----------
 src/modules/SX1268.cpp | 21 ++++++++-------------
 src/modules/SX126x.cpp | 20 ++++++++++----------
 src/modules/SX126x.h   |  1 -
 6 files changed, 53 insertions(+), 49 deletions(-)

diff --git a/src/modules/SX1261.cpp b/src/modules/SX1261.cpp
index 8d8cd0ab..06e37d73 100644
--- a/src/modules/SX1261.cpp
+++ b/src/modules/SX1261.cpp
@@ -1,6 +1,10 @@
 #include "SX1261.h"
 
-// note that this is untested (lacking the hardware) and based purely on the datasheet.
+SX1261::SX1261(Module* mod) 
+  : SX1262(mod) {
+
+}
+
 int16_t SX1261::setOutputPower(int8_t power) {  
   // check allowed power range
   if (!((power >= -17) && (power <= 14))) {
@@ -15,21 +19,19 @@ int16_t SX1261::setOutputPower(int8_t power) {
   }
 
   state = setOptimalLowPowerPaConfig(&power);
+  if (state != ERR_NONE) {
+    return(state);
+  }
 
   // set output power
   // TODO power ramp time configuration
-  if (state == ERR_NONE) {
-    state = SX126x::setTxParams(power);
+  state = SX126x::setTxParams(power);
+  if (state != ERR_NONE) {
+    return(state);
   }
 
   // restore OCP configuration
-  int16_t state2 = writeRegister(SX126X_REG_OCP_CONFIGURATION, &ocp, 1);
-
-  if (state != ERR_NONE) {
-    return state;
-  } else {
-    return state2;
-  }
+  return writeRegister(SX126X_REG_OCP_CONFIGURATION, &ocp, 1);
 }
 
 int16_t SX1261::setOptimalLowPowerPaConfig(int8_t* inOutPower)
@@ -40,7 +42,9 @@ int16_t SX1261::setOptimalLowPowerPaConfig(int8_t* inOutPower)
   }
   else {
     state = SX126x::setPaConfig(0x01, SX126X_PA_CONFIG_SX1261, 0x00);
-    *inOutPower -= 4;
+    // changing the PaConfig means output power is now scaled so we get 3 dB less than requested.
+    // see datasheet table 13-21 and comments in setOptimalHiPowerPaConfig.
+    *inOutPower -= 3;
   }
   return state;
 }
diff --git a/src/modules/SX1261.h b/src/modules/SX1261.h
index 4b1ce8fc..48667055 100644
--- a/src/modules/SX1261.h
+++ b/src/modules/SX1261.h
@@ -6,6 +6,8 @@
 #include "SX126x.h"
 #include "SX1262.h"
 
+//SX126X_CMD_SET_PA_CONFIG
+#define SX126X_PA_CONFIG_SX1261                       0x01
 
 // TODO: implement SX1261 class
 class SX1261 : public SX1262 {
@@ -15,10 +17,15 @@ class SX1261 : public SX1262 {
 
       \param mod Instance of Module that will be used to communicate with the radio.
     */
-    SX1261(Module* mod)
-      : SX1262(mod) {
-    }
+    SX1261(Module* mod);
 
+    /*!
+      \brief Sets output power. Allowed values are in range from -17 to 14 dBm.
+
+      \param power Output power to be set in dBm.
+
+      \returns \ref status_codes
+    */
     int16_t setOutputPower(int8_t power);
 
   private:
diff --git a/src/modules/SX1262.cpp b/src/modules/SX1262.cpp
index 94f94837..73731972 100644
--- a/src/modules/SX1262.cpp
+++ b/src/modules/SX1262.cpp
@@ -85,7 +85,7 @@ int16_t SX1262::setFrequency(float freq, bool calibrate) {
 
 int16_t SX1262::setOutputPower(int8_t power) {
   // check allowed power range
-  if (!(power >= -17 && power <= 22)) {
+  if (!((power >= -17) && (power <= 22))) {
     return(ERR_INVALID_OUTPUT_POWER);
   }
 
@@ -97,21 +97,20 @@ int16_t SX1262::setOutputPower(int8_t power) {
   }
 
   // this function sets the optimal PA settings 
-  // and scales our requested power based
+  // and adjusts power based on the PA settings chosen
+  // so that output power matches requested power.
   state = SX126x::setOptimalHiPowerPaConfig(&power);
+  if (state != ERR_NONE) {
+    return(state);
+  }
 
   // set output power
   // TODO power ramp time configuration
-  if (state == ERR_NONE) {
-    state = SX126x::setTxParams(power);
+  state = SX126x::setTxParams(power);
+  if (state != ERR_NONE) {
+    return(state);
   }
 
   // restore OCP configuration
-  int16_t state2 = writeRegister(SX126X_REG_OCP_CONFIGURATION, &ocp, 1);
-  
-  if (state != ERR_NONE) {
-    return state;
-  } else {
-    return state2;
-  }
+  return writeRegister(SX126X_REG_OCP_CONFIGURATION, &ocp, 1);
 }
diff --git a/src/modules/SX1268.cpp b/src/modules/SX1268.cpp
index a752c638..3ffb9a2a 100644
--- a/src/modules/SX1268.cpp
+++ b/src/modules/SX1268.cpp
@@ -78,9 +78,6 @@ int16_t SX1268::setFrequency(float freq, bool calibrate) {
 
 int16_t SX1268::setOutputPower(int8_t power) {
   // check allowed power range
-  // TODO with optimal PA config
-  // it's likely possible this could be expanded to go down to -17 just like for SX1262.
-  // but the datasheet doesn't explicitly state that and I don't have SX1268 unit to test with
   if(!((power >= -9) && (power <= 22))) {
     return(ERR_INVALID_OUTPUT_POWER);
   }
@@ -92,21 +89,19 @@ int16_t SX1268::setOutputPower(int8_t power) {
     return(state);
   }
 
-  // enable optimal PA
+  // enable optimal PA - this changes the value of power.
   state = SX126x::setOptimalHiPowerPaConfig(&power);
+  if (state != ERR_NONE) {
+    return(state);
+  }
 
   // set output power
   // TODO power ramp time configuration
-  if (state == ERR_NONE) {
-    state = SX126x::setTxParams(power);
+  state = SX126x::setTxParams(power);
+  if (state != ERR_NONE) {
+    return(state);
   }
 
   // restore OCP configuration
-  int16_t state2 = writeRegister(SX126X_REG_OCP_CONFIGURATION, &ocp, 1);
-
-  if (state != ERR_NONE) {
-    return state;
-  } else {
-    return state2;
-  }
+  return writeRegister(SX126X_REG_OCP_CONFIGURATION, &ocp, 1);
 }
diff --git a/src/modules/SX126x.cpp b/src/modules/SX126x.cpp
index 2e759226..3868e098 100644
--- a/src/modules/SX126x.cpp
+++ b/src/modules/SX126x.cpp
@@ -1127,29 +1127,29 @@ int16_t SX126x::setTxParams(uint8_t power, uint8_t rampTime) {
   return(SPIwriteCommand(SX126X_CMD_SET_TX_PARAMS, data, 2));
 }
 
-// set PA config for optimal consumption as described in section 13-21 of the datasheet:
-// the final column of Table 13-21 suggests that the value passed in SetTxParams 
-// is actually scaled depending on the parameters of setPaConfig.
-// Testing confirms this is approximately right
-int16_t SX126x::setOptimalHiPowerPaConfig(int8_t* inOutPower)
+// set PA config for optimal consumption as described in section 13-21 of the datasheet.
+int16_t SX126x::setOptimalHiPowerPaConfig(int8_t * inOutPower)
 {
+  // the final column of Table 13-21 suggests that the value passed in SetTxParams 
+  // is actually scaled depending on the parameters of setPaConfig.
+  // Testing confirms this is approximately right
   int16_t state;
   if (*inOutPower >= 21) {
     state = SX126x::setPaConfig(0x04, SX126X_PA_CONFIG_SX1262_8, SX126X_PA_CONFIG_HP_MAX/*0x07*/);
   }
   else if (*inOutPower >= 18) {
     state = SX126x::setPaConfig(0x03, SX126X_PA_CONFIG_SX1262_8, 0x05);
+    // datasheet instructs request 22 dBm for 20 dBm actual output power
     *inOutPower += 2;
-  }
-  else if (*inOutPower >= 15) {
+  } else if (*inOutPower >= 15) {
     state = SX126x::setPaConfig(0x02, SX126X_PA_CONFIG_SX1262_8, 0x03);
+    // datasheet instructs request 22 dBm for 17 dBm actual output power
     *inOutPower += 5;
-  }
-  else {
+  } else {
     state = SX126x::setPaConfig(0x02, SX126X_PA_CONFIG_SX1262_8, 0x02);
+    // datasheet instructs request 22 dBm for 14 dBm actual output power.
     *inOutPower += 8;
   }
-  // TODO investigate if better power efficiency can be achieved using undocumented even lower settings
   return state;
 }
 
diff --git a/src/modules/SX126x.h b/src/modules/SX126x.h
index 2bcd47ff..4c3dc61c 100644
--- a/src/modules/SX126x.h
+++ b/src/modules/SX126x.h
@@ -152,7 +152,6 @@
 //SX126X_CMD_SET_PA_CONFIG
 #define SX126X_PA_CONFIG_HP_MAX                       0x07
 #define SX126X_PA_CONFIG_PA_LUT                       0x01
-#define SX126X_PA_CONFIG_SX1261                       0x01
 #define SX126X_PA_CONFIG_SX1262_8                     0x00
 
 //SX126X_CMD_SET_RX_TX_FALLBACK_MODE