Skip to content
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

Directly revert back to MAX_TX_POWER on AdrAckCounter >= ADR_ACK_LIMIT? #723

Closed
brocaar opened this issue Apr 11, 2019 · 5 comments
Closed

Comments

@brocaar
Copy link

brocaar commented Apr 11, 2019

    if( adrNext->AdrEnabled == true )
    {
        if( datarate == EU868_TX_MIN_DATARATE )
        {
            *adrAckCounter = 0;
            adrAckReq = false;
        }
        else
        {
            if( adrNext->AdrAckCounter >= EU868_ADR_ACK_LIMIT )
            {
                adrAckReq = true;
                // \/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/
                txPower = EU868_MAX_TX_POWER;
            }
            else
            {
                adrAckReq = false;
            }
            if( adrNext->AdrAckCounter >= ( EU868_ADR_ACK_LIMIT + EU868_ADR_ACK_DELAY ) )
            {
                if( ( adrNext->AdrAckCounter % EU868_ADR_ACK_DELAY ) == 1 )
                {
                    // Decrease the datarate
                    getPhy.Attribute = PHY_NEXT_LOWER_TX_DR;
                    getPhy.Datarate = datarate;
                    getPhy.UplinkDwellTime = adrNext->UplinkDwellTime;
                    phyParam = RegionEU868GetPhyParam( &getPhy );
                    datarate = phyParam.Value;

                    if( datarate == EU868_TX_MIN_DATARATE )
                    {
                        if( adrNext->UpdateChanMask == true )
                        {
                            // Re-enable default channels
                            ChannelsMask[0] |= LC( 1 ) + LC( 2 ) + LC( 3 );
                        }
                    }
                }
            }
        }

txPower = EU868_MAX_TX_POWER;

Shouldn't this happen only when the device is decreasing the data-rate?

How I understand the ADRAckReq is that it is there to force the NS to send a downlink, even when it doesn't have any downlink data in its queue / mac-commands to send so that the device can confirm it is still connected. I don't believe reverting the TX power to its max is the correct behavior at this step.

It would make sense within if( adrNext->AdrAckCounter >= ( EU868_ADR_ACK_LIMIT + EU868_ADR_ACK_DELAY ) ) I believe as this is where the device starts lowering its data-rate.

@mluis1
Copy link
Contributor

mluis1 commented Apr 12, 2019

Your understanding of the specification is correct.

The network server has just to perform a downlink to the end-device to confirm that the server and the end-device are seeing each other.

Setting the txPower to the maximum value is something that is not specified on LoRaWAN specifications below or equal to 1.0.3.
This has been implemented upon request from some network server operators. They wanted the end-device to first set the radio Tx power to its maximum and then at the other steps to start decreasing the data rate.

From the description on referenced 389 issue I think that this will have a small influence on the end-device power consumption.

  • In case the network server receives the uplink and then the end-device receives a downlink. Then the network server ADR algorithm will anyway send a LinkAdrReq mac command as it will notice that something changed. (may take a few uplinks depending on the ADR algorithm being used)
  • In case the end-device doesn't receive a downlink there is no impact as the txPower must be raised anyway.

In LoRaWAN specification 1.1.0 the increase of the txPower has been defined and the feature/5.0.0 branch already implements it.
image
https://github.com/Lora-net/LoRaMac-node/blob/feature/5.0.0/src/mac/LoRaMacAdr.c#L109 onward.

Please note that the LoRaWAN 1.1.0 ADRACKReq bit behavior is being back ported to the future LoRaWAN 1.0.4 specification.

As of today I don't see the necessity to change the current implementation. But, if there is a strong demand to do so I think that your proposition looks to be a good compromise. We just need to verify if this doesn't break the LoRaWAN certification.

@htdvisser
Copy link

This line can actually be quite problematic if the network server assumes (as it should) that end devices follow the specification.

541 [...] After ADR_ACK_LIMIT uplinks
542 (ADR_ACK_CNT >= ADR_ACK_LIMIT) without any downlink response, it sets the ADR
543 acknowledgment request bit (ADRACKReq). The network is required to respond with a
544 downlink frame within the next ADR_ACK_DELAY frames, any received downlink frame
545 following an uplink frame resets the ADR_ACK_CNT counter. [...]

Nothing is said here about stepping up the transmit power here, so this behavior is not compliant with the specification. What is specified is that the device must step up the transmit power after ADR_ACK_LIMIT + ADR_ACK_DELAY uplinks:

547 [...] If no reply is received within the
548 next ADR_ACK_DELAY uplinks (i.e., after a total of ADR_ACK_LIMIT +
549 ADR_ACK_DELAY), the end-device MUST try to regain connectivity by first stepping up the
550 transmit power to default power if possible then switching to the next lower data rate that
551 provides a longer radio range. The end-device MUST further lower its data rate step by step
552 every time ADR_ACK_DELAY is reached. Once the device has reached the lowest data
553 rate, it MUST re-enable all default uplink frequency channels.

So perhaps the line @brocaar pointed out is just in the wrong place and should be moved to below if( adrNext->AdrAckCounter >= ( EU868_ADR_ACK_LIMIT + EU868_ADR_ACK_DELAY ) )?


The reason that this is problematic is that the ADR algorithm in @TheThingsNetwork's stack (and hopefully other stacks) expects that end devices to follow the behavior described in the specification above, and therefore assumes that the end device did not immediately change the transmit power to MAX when an uplink with AdrAckReq=1 is received by the network server. Therefore, the ADR optimization algorithm is performed with the known parameters of the end device (i.e. the TxPower before the device decided to set it to MAX). This will result in an incorrect optimization, as the measured link margin was not achieved with the expected/known TxPower, but with the MAX TxPower. The network server doesn't know that, and may optimize the TxPower of the end device relative to the known TxPower, possibly sending it to a too low TxPower. This may break the connectivity of the device, and is obviously not what we want.

@mluis1
Copy link
Contributor

mluis1 commented Jun 14, 2019

@htdvisser Thanks for the feedback.

Generally, I agree with your remarks. but, in 1.0.x LoRaWAN specifications the transmit power is not mentioned at all. As said on my previous comment the addition of the transmit power control was a request from some LNS providers.

If we want to exactly follow the 1.0.3 specification actually we should completely remove the transmit power control. But, there is the risk to have end-devices not being able to reach the LNS in some conditions.

We didn't want to make the @brocaar proposed change because we were afraid to break the current LoRaWAN certification process.
So, in order to have everyone happy I propose to apply the proposed change to be just after the if( adrNext->AdrAckCounter >= ( EU868_ADR_ACK_LIMIT + EU868_ADR_ACK_DELAY ) ) and then verify that the certification doesn't get broken.

As a side note, the text that you reference doesn't look to correspond to LoRaWAN 1.0.3 specification.

The 1.0.3 specification doesn't mention the transmit power at all. Neither when the LNS performs a downlink or when it doesn't.

image

The text that you reference looks more to the one in LoRaWAN 1.1.x specifications. In plus these specifications provide an example on how it should work.

@mluis1 mluis1 reopened this Jun 14, 2019
@htdvisser
Copy link

Since 1.0.4 is still under NDA, I couldn't quote its text directly. However, as you already wrote, this part is backported from 1.1, so I quoted the 1.1 text.

Since neither the current implementation (switching to max tx power after ADR_ACK_LIMIT) nor the proposed change (switching to max tx power after ADR_ACK_LIMIT + ADR_ACK_DELAY) is explicitly described in pre-1.0.4 specifications, the most correct solution (from a certification perspective) would be to completely remove the line for pre-1.0.4 certification and move the line (as proposed) for 1.0.4+ certification.

@mluis1
Copy link
Contributor

mluis1 commented Jun 14, 2019

The reported issue here was on master branch which implements 1.0.2 specification.
The develop branch implements 1.0.3 specification. As the only big change from the 2 specifications was just the addition of the ClassB, this issue is present on both.

I think that the proposed change shouldn't have an impact on the certification and it avoids loosing devices at a small cost. I only see advantages by implementing the transmission power control, that's why this has been included in recent versions of the specification.

The feature/5.0.0 branch implements already the 1.1.x behavior when connected to a 1.1.x LNS. If connected to a 1.0.x LNS the used behavior is the 1.0.x.

We will start working on 1.0.4 specification in the coming weeks. This new version has the same exact implementation as the 1.1.x versions of the specification.

@mluis1 mluis1 added this to the Release Version 4.4.2 milestone Jun 17, 2019
mluis1 added a commit that referenced this issue Jun 17, 2019
…>= ( adrNext->AdrAckLimit + adrNext->AdrAckDelay ) )`

The Tx power control is not specified into LoRaWAN specifications 1.0.3 and below.
Although its control is advisable in order to avoid loosing connection in some conditions.
This addition shouldn't have any impact on ADR algorithms nor on the certification process.
@mluis1 mluis1 closed this as completed Jun 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants