[Feature Request] enableGNSS - using combined constellations

[hybridOL]

Subject of the issue

The functions for getting/setting the GNSS enabling take a variable of type enum sfe_ublox_gnss_ids_e
However, the implementation indicates a little different use with combined values of this type. So if you want to enable GPS and Galileo you would send SFE_UBLOX_GNSS_ID_GPS | SFE_UBLOX_GNSS_ID_GALILEO but this will result in a warning as this combined value is not a member of the enum. So I'd suggest to change the type to int. As the functions check the actual values anyway, it would not matter if illegal combinations are sent.
However, the isGNSSenabled function does check for all values being set in the parameter, but the result only shows if at least one of those values is enabled. I guess that's not meant, as in that case the true value could immediately be returned. Instead, you should return false immediately if one of the values is not set. That way, all values are properly checked for all being activated.

There is no specific board or module required, it's just being based on code review.

Expected behavior

OR combined enum values should be accepted, as code already deals with it
check for being enabled should return true only if all OR combined GNSS types are activated

Actual behavior

compiler raises warning/error with OR combination
Check function returns true if at least one type is activated

[PaulZC]

Hi @hybridOL ,

enableGNSS is only able to enable or disable a single constellation at a time. ORing combinations of the enum is actually illegal. Please study the code for enableGNSS and you will see what I mean. Specifically, this line would fail with combined values:

SparkFun_u-blox_GNSS_Arduino_Library/src/SparkFun_u-blox_GNSS_Arduino_Library.cpp

Line 8289 in 3416a31

if (payloadCfg[(block * 8) + 4] == (uint8_t)id) // Check the gnssId for this block. Do we have a match?

With v3 of the library, the enum is even more important as it controls which Configuration Interface Key is selected for each constellation. Again combined values are illegal.

Best wishes,
Paul

[hybridOL]

Yeah, you're right about the check - also the numbers are not yet combinable. Still, this command is quite large nad requires a 500ms pause after execution. So it would be desirable to have a multile system enable command.
The shown line could be replaced by
if ((1<<payloadCfg[(block * 8) + 4]) & (uint8_t)id) // Check the gnssId for this block. Do we have a match?
Checking the configuration could be made by doing the same shift and passing that value to the array or whatever is used.

[PaulZC]

Good point about the execution time...

For modules that support the Configuration Interface (F9, M10), this is easy:

  bool setValueSuccess = true;

  //Begin with newCfgValset
  setValueSuccess &= myGNSS.newCfgValset(); // Defaults to configuring the setting in Flash, RAM and BBR
  //setValueSuccess &= myGNSS.newCfgValset(VAL_LAYER_RAM); //Set the following settings in RAM only

  // Add KeyIDs and Values
  setValueSuccess &= myGNSS.addCfgValset8(UBLOX_CFG_SIGNAL_GPS_ENA, 1); //Enable GPS
  setValueSuccess &= myGNSS.addCfgValset8(UBLOX_CFG_SIGNAL_SBAS_ENA, 1); //Enable SBAS
  setValueSuccess &= myGNSS.addCfgValset8(UBLOX_CFG_SIGNAL_GAL_ENA, 0); //Disable Galileo
  setValueSuccess &= myGNSS.addCfgValset8(UBLOX_CFG_SIGNAL_BDS_ENA, 0); //Disable BeiDou
  setValueSuccess &= myGNSS.addCfgValset8(UBLOX_CFG_SIGNAL_QZSS_ENA, 0); //Disable QZSS
  setValueSuccess &= myGNSS.addCfgValset8(UBLOX_CFG_SIGNAL_GLO_ENA, 1); //Enable GLONASS

  // Send the packet using sendCfgValset
  setValueSuccess &= myGNSS.sendCfgValset();

  if (setValueSuccess == true)
  {
    Serial.println("Values were successfully set");
  }
  else
    Serial.println("Value set failed");

But for the M8, we are stuck with CFG-GNSS...

The gnssId values are defined by u-blox. We are stuck with those. But I would be OK with adding a new set of const ints - probably 16-bit just to leave room for expansion:

const uint16_t SFE_UBLOX_ENABLE_GNSS_GPS = 1 << 0;
const uint16_t SFE_UBLOX_ENABLE_GNSS_SBAS = 1 << 1;
const uint16_t SFE_UBLOX_ENABLE_GNSS_GALILEO = 1 << 2;
const uint16_t SFE_UBLOX_ENABLE_GNSS_BEIDOU = 1 << 3;
const uint16_t SFE_UBLOX_ENABLE_GNSS_IMES = 1 << 4;
const uint16_t SFE_UBLOX_ENABLE_GNSS_QZSS = 1 << 5;
const uint16_t SFE_UBLOX_ENABLE_GNSS_GLONASS = 1 << 6;

And then add a new method containing your code:

bool enableGNSSByID(uint16_t gnssIDs, uint16_t maxWait = defaultMaxWait); //Enable these GNSS, disable all others

Another way could be:

Add a terminator to the GNSS ID enum:

enum sfe_ublox_gnss_ids_e
{
  SFE_UBLOX_GNSS_ID_GPS,
  SFE_UBLOX_GNSS_ID_SBAS,
  SFE_UBLOX_GNSS_ID_GALILEO,
  SFE_UBLOX_GNSS_ID_BEIDOU,
  SFE_UBLOX_GNSS_ID_IMES,
  SFE_UBLOX_GNSS_ID_QZSS,
  SFE_UBLOX_GNSS_ID_GLONASS,
  SFE_UBLOX_GNSS_ID_END = 255
};

Then add a new method which walks through the gnssIDs until it reaches END:

bool enableGNSSByID(uint8_t *gnssIDs, uint16_t maxWait = defaultMaxWait); //Enable these GNSS, disable all others

The code would look something like:

uint8_t enableThese[] = { SFE_UBLOX_GNSS_ID_GPS, SFE_UBLOX_GNSS_ID_GLONASS, SFE_UBLOX_GNSS_ID_END };
if (myGNSS.enableGNSSByID(enableThese))

Let me know if this would work for you.

Best wishes,
Paul

[hybridOL]

Yeah, I'm using M8. The two enums will probably have the problem that the new function would accept the consecutive values and would create horrible confusion. The array is a nice idea. The interface is unique and the enum stays backward compatible. The list is also nicely readable and statically configurable. I'd vote for that one, can come up with some code over the weekend.

[PaulZC]

How about this:

Add the enum terminator, but make it consecutive:

enum sfe_ublox_gnss_ids_e
{
  SFE_UBLOX_GNSS_ID_GPS,
  SFE_UBLOX_GNSS_ID_SBAS,
  SFE_UBLOX_GNSS_ID_GALILEO,
  SFE_UBLOX_GNSS_ID_BEIDOU,
  SFE_UBLOX_GNSS_ID_IMES,
  SFE_UBLOX_GNSS_ID_QZSS,
  SFE_UBLOX_GNSS_ID_GLONASS,
  //< Insert possible future GNSS here>
  SFE_UBLOX_GNSS_ID_SIZE
};

Define the configure method as the following. To make it memory-safe, don't provide a default for numGNSS:

bool enableGNSSByID(uint8_t *gnssIDs, uint8_t numGNSS, uint16_t maxWait = defaultMaxWait);

The code would look like:

uint8_t gnss[SFE_UBLOX_GNSS_ID_SIZE] = { 1, 1, 0, 0, 0, 1, 1 };
myGNSS.enableGNSSByID(gnss, SFE_UBLOX_GNSS_ID_SIZE);

You could also then do things like:

gnss[SFE_UBLOX_GNSS_ID_GPS] = 1;
gnss[SFE_UBLOX_GNSS_ID_GLONASS] = 0;

The isGNSSenabled method could use the same array. To make it memory-safe, don't provide a default for numGNSS:

bool isGNSSenabledByID(uint8_t *gnssIDs, uint8_t numGNSS, uint16_t maxWait = defaultMaxWait);

You could then use similar methods to set the maxTrkCh and sigCfgMask

[hybridOL]

I think the enum terminator is not necessary in this version, correct? Also, should the isEnabled function return an array with enable/disable being set for each GNSS, or should we test all GNSS which are set in the array and return a combined bool of it? I guess returning the array with updated values would be most versatile approach, but of course requires additional checks in user code. Will do some tests with the chip and provide the code later

[PaulZC]

The terminator is not strictly necessary, but it makes it easy to define the correct size for the array.

If you have time, please do submit another pull request. I am happy to write the new code but I am busy with another major project. It may be ~two weeks before I have time to work on it.