Set MAC state for devices with active session #14

neoaggelos · 2020-12-23T23:50:27Z

Summary

References https://github.com/TheThingsIndustries/lorawan-stack/issues/2417

Changes

For ~~OTAA~~ devices with a session, set ~~current parameters instead of~~ MAC state. ~~This is only relevant for the Rx1 Delay for devices exported from TTNv2~~

Testing

Test with devices from TTN

OTAA device, without a session -> Sets mac_settings.rx1_delay
OTAA device with session -> Sets mac_state.current_parameters.rx1_delay
ABP devices -> Sets mac_settings.rx1_delay

Checklist

Scope: The referenced issue is addressed, there are no unrelated changes.
Documentation: Relevant documentation is added or updated.
Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.

rvolosatovs · 2020-12-24T14:43:39Z

pkg/source/ttnv2/source.go

+	if deviceHasSession && deviceSupportsJoin {
+		v3dev.MACState = &ttnpb.MACState{
+			DeviceClass:    ttnpb.CLASS_A,
+			LoRaWANVersion: ttnpb.MAC_V1_0_2,
+			CurrentParameters: ttnpb.MACParameters{
+				Rx1Delay: ttnpb.RX_DELAY_1,
+			},
+		}
+	} else {
+		v3dev.MACSettings.Rx1Delay = &ttnpb.RxDelayValue{
+			Value: ttnpb.RX_DELAY_1,
+		}
+	}


Suggested change

if deviceHasSession && deviceSupportsJoin {

v3dev.MACState = &ttnpb.MACState{

DeviceClass: ttnpb.CLASS_A,

LoRaWANVersion: ttnpb.MAC_V1_0_2,

CurrentParameters: ttnpb.MACParameters{

Rx1Delay: ttnpb.RX_DELAY_1,

},

}

} else {

v3dev.MACSettings.Rx1Delay = &ttnpb.RxDelayValue{

Value: ttnpb.RX_DELAY_1,

}

}

if deviceHasSession {

v3dev.MACState = &ttnpb.MACState{

DeviceClass: ttnpb.CLASS_A,

LoRaWANVersion: ttnpb.MAC_V1_0_2,

CurrentParameters: ttnpb.MACParameters{

Rx1Delay: ttnpb.RX_DELAY_1,

},

}

}

if !deviceSupportsJoin {

v3dev.MACSettings.Rx1Delay = &ttnpb.RxDelayValue{

Value: ttnpb.RX_DELAY_1,

}

}

rvolosatovs · 2020-12-24T14:47:29Z

If session is set, then a complete mac-state should be set as well, Network Server should not allow setting just session, for example, and not setting mac-state as well.

In fact, I think the best approach is CLI using https://github.com/TheThingsNetwork/lorawan-stack/blob/b4c8dcda48ba9b43e383a7e09e909c4607cbd565/pkg/networkserver/mac/utils.go#L548-L601 to generate a MAC state, doing all the adjustments for v2 and then setting that

rvolosatovs

This looks good, but the tool should be careful with the field paths sent to NS - if the tool sends top-level mac_state, then the created device will end up with a corrupted MAC state and will be unusable

rvolosatovs · 2020-12-24T15:19:10Z

And, to reiterate, I think the cleanest approach would be:

Generate new MAC state
Apply all modifications to it according to v2 specifics
Send the whole mac_state with top-level mac_state field path specified

htdvisser

LGTM

rvolosatovs

A possible improvement is to not set mac-state.desired-parameters, such that v3 NS would compute that by itself and modify the state accordingly, but I'm not sure we should do that.
For example, that would mean that immediately after migration v3 NS would try to reconfigure device to use RX1 Delay of 5 seconds(v3 default) via MAC commands, @johanstokking @htdvisser - what do you think?
IMO it's safest not to do that and just let the device stay with exact same configuration as v2 to minimize chance of potential issues. Users can set mac-state.desired-parameters.rx1-delay if necessary

pkg/source/ttnv2/config.go

johanstokking · 2020-12-28T13:50:27Z

For example, that would mean that immediately after migration v3 NS would try to reconfigure device to use RX1 Delay of 5 seconds(v3 default) via MAC commands, @johanstokking @htdvisser - what do you think?

Yeah good point.

What are the other settings that we should consider, besides RX1 delay?

As for this particular instance, I do think that we should reconfigure the RX1 delay to TTS' default.

neoaggelos · 2020-12-29T09:53:58Z

Updated according to our discussion with @rvolosatovs. Will proceed with testing after NS supports importing device sessions.

johanstokking · 2020-12-30T11:46:15Z

pkg/source/ttnv2/source.go

@@ -210,5 +209,8 @@ func (s *Source) RangeDevices(appID string, f func(source.Source, string) error)

 // Close implements the Source interface.
 func (s *Source) Close() error {
-	return s.client.Close()
+	if s.client != nil {


I don't really like the fact that we lazy-initialize client with the first application ID that is seen by Source. Is there a good reason for that?

Lazy initialization is removed as part of #13

rvolosatovs · 2021-01-04T17:11:00Z

Blocked by TheThingsNetwork/lorawan-stack#2472

rvolosatovs · 2021-01-19T20:10:29Z

Blocking https://github.com/TheThingsIndustries/lorawan-stack/issues/2417

rvolosatovs · 2021-01-20T19:13:31Z

pkg/source/ttnv2/source.go

+		v3dev.MACState.DeviceClass = ttnpb.CLASS_A
+		v3dev.MACState.LoRaWANVersion = ttnpb.MAC_V1_0_2
+		v3dev.MACState.CurrentParameters.Rx1Delay = ttnpb.RX_DELAY_1
+		v3dev.MACState.DesiredParameters = ttnpb.MACParameters{}


You should not need to be doing this. Instead, this should be covered by the field mask.
Only relevant fields in mac_state.current_parameters and in top-level mac_state should be specified

johanstokking · 2021-01-21T09:14:40Z

@neoaggelos if this is ready for review, request them.

neoaggelos · 2021-01-21T09:21:50Z

I am currently testing against the changes in TheThingsNetwork/lorawan-stack#3680, will request review when ready.

rvolosatovs

I don't see the fieldmask in the diff and don't know where to find it, but I assume that is correct

rvolosatovs · 2021-02-16T17:05:29Z

FYI, TheThingsNetwork/lorawan-stack#3680 is still not merged.

neoaggelos added the ui/cli label Dec 23, 2020

neoaggelos added this to the December 2020 milestone Dec 23, 2020

neoaggelos requested a review from johanstokking as a code owner December 23, 2020 23:50

neoaggelos self-assigned this Dec 23, 2020

neoaggelos requested a review from rvolosatovs December 23, 2020 23:50

rvolosatovs suggested changes Dec 24, 2020

View reviewed changes

rvolosatovs changed the title ~~Set current parameters instead of MAC settings for active OTAA devices~~ Set MAC state for devices with active session Dec 24, 2020

neoaggelos force-pushed the feature/otaa-mac-state branch from 4f739fe to e5caad0 Compare December 24, 2020 14:50

neoaggelos requested review from rvolosatovs and removed request for rvolosatovs December 24, 2020 14:50

rvolosatovs approved these changes Dec 24, 2020

View reviewed changes

neoaggelos requested a review from htdvisser as a code owner December 24, 2020 15:21

htdvisser approved these changes Dec 28, 2020

View reviewed changes

rvolosatovs approved these changes Dec 28, 2020

View reviewed changes

pkg/source/ttnv2/config.go Show resolved Hide resolved

johanstokking reviewed Dec 30, 2020

View reviewed changes

johanstokking modified the milestones: December 2020, January 2021 Jan 4, 2021

rvolosatovs added the status/blocked label Jan 4, 2021

This was referenced Jan 19, 2021

Allow creation of devices with preexisting MAC state in Network Server TheThingsNetwork/lorawan-stack#2472

Closed

Allow session import in Network Server TheThingsNetwork/lorawan-stack#3680

Merged

rvolosatovs added status/blocking/issue prio/medium labels Jan 19, 2021

neoaggelos force-pushed the feature/otaa-mac-state branch 2 times, most recently from 6902562 to 7f3518e Compare January 20, 2021 15:24

rvolosatovs suggested changes Jan 20, 2021

View reviewed changes

neoaggelos force-pushed the feature/otaa-mac-state branch from 7f3518e to e3988a4 Compare January 21, 2021 00:50

rvolosatovs approved these changes Jan 21, 2021

View reviewed changes

cli: Export OTAA devices with existing MAC state and session

5aa1a1a

neoaggelos force-pushed the feature/otaa-mac-state branch from e3988a4 to 5aa1a1a Compare February 12, 2021 14:26

neoaggelos merged commit 7418528 into master Feb 12, 2021

rvolosatovs deleted the feature/otaa-mac-state branch February 16, 2021 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set MAC state for devices with active session #14

Set MAC state for devices with active session #14

neoaggelos commented Dec 23, 2020 •

edited by rvolosatovs

Loading

rvolosatovs Dec 24, 2020

rvolosatovs commented Dec 24, 2020 •

edited

Loading

rvolosatovs left a comment

rvolosatovs commented Dec 24, 2020

htdvisser left a comment

rvolosatovs left a comment

johanstokking commented Dec 28, 2020

neoaggelos commented Dec 29, 2020

johanstokking Dec 30, 2020

neoaggelos Jan 12, 2021

rvolosatovs commented Jan 4, 2021 •

edited

Loading

rvolosatovs commented Jan 19, 2021

rvolosatovs Jan 20, 2021

johanstokking commented Jan 21, 2021

neoaggelos commented Jan 21, 2021

rvolosatovs left a comment

rvolosatovs commented Feb 16, 2021

Set MAC state for devices with active session #14

Set MAC state for devices with active session #14

Conversation

neoaggelos commented Dec 23, 2020 • edited by rvolosatovs Loading

Summary

Changes

Testing

Checklist

rvolosatovs Dec 24, 2020

Choose a reason for hiding this comment

rvolosatovs commented Dec 24, 2020 • edited Loading

rvolosatovs left a comment

Choose a reason for hiding this comment

rvolosatovs commented Dec 24, 2020

htdvisser left a comment

Choose a reason for hiding this comment

rvolosatovs left a comment

Choose a reason for hiding this comment

johanstokking commented Dec 28, 2020

neoaggelos commented Dec 29, 2020

johanstokking Dec 30, 2020

Choose a reason for hiding this comment

neoaggelos Jan 12, 2021

Choose a reason for hiding this comment

rvolosatovs commented Jan 4, 2021 • edited Loading

rvolosatovs commented Jan 19, 2021

rvolosatovs Jan 20, 2021

Choose a reason for hiding this comment

johanstokking commented Jan 21, 2021

neoaggelos commented Jan 21, 2021

rvolosatovs left a comment

Choose a reason for hiding this comment

rvolosatovs commented Feb 16, 2021

neoaggelos commented Dec 23, 2020 •

edited by rvolosatovs

Loading

rvolosatovs commented Dec 24, 2020 •

edited

Loading

rvolosatovs commented Jan 4, 2021 •

edited

Loading