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

Integrating LoRaWAN feature into Mbed-OS #5590

Closed

Conversation

Projects
None yet
9 participants
@hasnainvirk
Copy link
Contributor

commented Nov 27, 2017

Description

This PR adds LoRaWAN protocol support in Mbed-OS.
Radio drivers and the business application will be out of the tree.

Status

READY

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2017

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:LORAWAN_FEATURE_BRANCH branch Nov 27, 2017

features/netsocket/LoRaWANBase.h Outdated
#define LORAWAN_BASE_H_

#include "lorawan/system/lorawan_data_structures.h"
#include "mbed.h"

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Nov 27, 2017

Member

What headers this file needs, should not pull in mbed.h, but be more specific with inclusion. I've seen this in couple of files here.

features/netsocket/LoRaRadio.h Outdated
/**
* Sets the reception parameters.
*
* \param modem The radio modem to be used [0: FSK, 1: LoRa].

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Nov 27, 2017

Member

should we be consistent, our doxy in this codebase uses the other style @param, Not big deal

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Nov 28, 2017

Author Contributor

Yeah. I think at some point I had all Arm files with @param sort of doxy flags. LoRaMac.cpp uses a different fashion, and I think it just got mixed at some point. We squashed from 65 commits to 7 commits :) , so you can have an idea what we went through.

TESTS/lorawan/application_api/main.cpp Outdated

using namespace utest::v1;

#define MBED_CONF_LORA_PHY 0

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Nov 27, 2017

Member

Should this default value be set via config, rather than here?

TESTS/lorawan/application_api/main.cpp Outdated
LORA_DIO5, NC, NC, LORA_TXCTL, LORA_RXCTL, NC, NC);
#endif
#if TARGET_K64F
SX1276_LoRaRadio Radio(D11, D12, D13, D10, A0,

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Nov 27, 2017

Member

K64F does not have defined pins LORA_MOSI, etc? Can be done via config? Same for disco below? to have one object in this file rather than ifdef else per target?

Same could be applied to specific header file per target ?

features/lorawan/LoRaWANStack.cpp Outdated
mlme_confirm_handler(&lora_mlme_confirm);
}

void LoRaWANStack::set_lora_event_cb(Callback<void(lora_events_t)> event_cb)

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Nov 27, 2017

Member

Is this attach() method ?

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Nov 28, 2017

Author Contributor

It is used to post events to user. User provided callback function.

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Nov 28, 2017

Member

In our API we use attach(), this is not the case here?

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Nov 28, 2017

Author Contributor

Actual API visible to user is lora_event_callback() defined in LoRaWANInterface class.
This name was suggested by Jan and Sam. I am not keen on any name :), if majority wants it to be attach, we can name it attach. Let's wait for what others say.

features/lorawan/LoRaWANInterface.cpp Outdated

#include "lorawan/LoRaWANInterface.h"

#define STK_OBJ LoRaWANStack::get_lorawan_stack()

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Nov 27, 2017

Member

Shall we just use inlined local function rather macro for this

features/lorawan/LoRaWANStack.h Outdated
*
* @return The number of bytes sent, a negative error code on failure.
*/
int16_t handle_tx(uint8_t port, const uint8_t* data,

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Nov 27, 2017

Member

handle_tx instead of write ? same for handle_rx

the doc above does seem to be trimmed message is sent, for example:

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Nov 28, 2017

Author Contributor

It's not exactly write(). This initiates a process for TX. If duty cycle is off, it sends immidiately. If duty cycle is on, it only schedules the transmission. In my opinion handle_tx() is better suited name. Comment is right, it returns the number of bytes sent from your message.

features/lorawan/LoRaWANStack.h Outdated
*
* In response to the user call for disconnection, the stack shuts down itself.
*/
void shutdown_lorawan();

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Nov 27, 2017

Member

only shutdown not sufficient, suffix needed here?

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Nov 28, 2017

Author Contributor

you mean prefix ? It does have a suffix 'lorawan'. That what it is, it shuts down lorawan protocol i.e., the mac layer.

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Nov 28, 2017

Member

Yes, prefix. if I invoke lorawanstack.shutdown() if that is not already explicit that what is happening.

features/lorawan/LoRaWANStack.h Outdated
void mcps_indication(McpsIndication_t *mcps_indication);
void mlme_confirm(MlmeConfirm_t *mlme_confirm);

lora_mac_status_t mlme_request_handler(lora_mac_mlme_req_t *mlme_request);

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Nov 27, 2017

Member

Should private methods be documented also? Might help developers.

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Nov 28, 2017

Author Contributor

Yeah, I will put something there.

features/netsocket/LoRaWANBase.h Outdated
#include "lorawan/system/lorawan_data_structures.h"
#include "mbed.h"

class LoRaWANBase {

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Nov 27, 2017

Member

no namespace, at least mbed should be

@0xc0170

This comment has been minimized.

Copy link
Member

commented Nov 27, 2017

Please review jenkins CI failures

@janjongboom

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2017

I haven't looked at this in-depth, but this seems weird:

        "tx-max-size": {
            "help": "User application data buffer maximum size, default: 64, MAX: 255",
            "value": 64
        },

Max TX size depends on spread factor chosen, so why have this here? Also TX can never be 255 bytes, max is 224 (give or take).

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2017

@janjongboom The phy layer sets a limit to max size which is 255 bytes. So that is what our buffer size is.
Before sending, stack checks according to the data rate (conversely speaking SF), of the channel selected, that how many bytes it can actually transmit and then it will send that many bytes. Then we tell the application that these many bytes from your data has been sent. And the user sends again the remaining data

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:LORAWAN_FEATURE_BRANCH branch 2 times, most recently Nov 28, 2017

Adding base class for all LoRa radio drivers
All Mbed-OS drivers for LoRa radio devices must implement
this pure virtual class in order to be compliant with Mbed-OS
applications.

This class comes loaded with all necessary data structures.
The implementations of this class can come out of tree.

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:LORAWAN_FEATURE_BRANCH branch Nov 28, 2017

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2017

@0xc0170 It's rather weird. CI is building properly for every other target except NUMAKER_PFM_NUC472. Do you have any idea why this is happening ?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Nov 28, 2017

@0xc0170 It's rather weird. CI is building properly for every other target except NUMAKER_PFM_NUC472. Do you have any idea why this is happening ?

When you check the log (first indication) you would see compiler errors, same as other indications, for instance ./mbed-os/features/lorawan/lorastack/mac/LoRaMac.cpp:2827:5: error: 'LoRaMacInitializationTime' was not declared in this scope

@0xc0170 0xc0170 requested review from sg-, janjongboom and AnttiKauppila Nov 28, 2017

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2017

@0xc0170 Yes I checked that. Why it is declared for other targets then ?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Nov 28, 2017

@0xc0170 Yes I checked that. Why it is declared for other targets then ?

@tommikas Can you help @hasnainvirk to reproduce locally the failures ?

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2017

@0xc0170 For some reason, NUMAKER_PFM_NUC472 does not create object files for the items in lorawan/system/ and lorawan/lorastack/mac .
I don't understand why.

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2017

@0xc0170 I can reproduce that locally. I have tried at least 4 different targets with blinky example. Only NUMAKER_ fails.

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2017

@0xc0170 Found the issue. timer.h had a header inclusion flag which was exactly like the NUMAKER_'s flag, i.e., TIMER_H

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:LORAWAN_FEATURE_BRANCH branch Nov 28, 2017

@0xc0170 0xc0170 requested a review from SenRamakri Nov 28, 2017

@kivaisan

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

About timer.cpp/.h. Would it be better to rename these timer sources as more specific name (LoRaTimer?) ? This can now conflict with mbed os' Timer.cpp.

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2017

@kivaisan I like that idea. With the current naming, there will be no problem even in Windows OS because we use qualified paths. But still I like the idea to rename these.

@tommikas

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

The qualified paths don't help with the inclusion guards either.

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2017

@tommikas inclusion guards was a different story. Already dealt with.

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:LORAWAN_FEATURE_BRANCH branch Nov 29, 2017

@0xc0170 0xc0170 requested a review from AnotherButler Nov 29, 2017

@0xc0170

This comment has been minimized.

Copy link
Member

commented Nov 29, 2017

@AnotherButler new API

@0xc0170

This comment has been minimized.

Copy link
Member

commented Nov 29, 2017

@hasnainvirk Can you please share here what files are user facing API? is it all in the root directory (https://github.com/hasnainvirk/mbed-os/tree/5b59851610027ba43101537c8b58e947ebc42bd1/features/lorawan), the rest are internal details? What is being exposed to a user (I would like to check what we are pulling to mbed namespace, types, API, all).

I can then edit my review.

hasnainvirk added some commits Nov 27, 2017

Adding base class for LoRaWAN interfaces
All network interfaces for LoRaWAN protocol must implement this
class. In order to be compatible with Mbed-OS applications, any
implementation of this class must use the data structures and
Mbed-OS timers provided.

lorawan_data_structures may look repetitive but this is essential
as we have a plan to use a reference implementation for LoRaWAN mac
layer from Semtech. Some of the data structures provide seemless
transition from semtech implementation (as MAC layer) to the Mbed-OS
control layers above.

features/lorawan/lorastack is the placeholder for future items like mac and
phy layers. system/ will contain all the common bits.
Adding PHY layer for LoRaWAN
LoRaPHY is the abstract class for the LoRa PHY layer which governs
the LoRaRadio and provides some common functionality to all regional
implementations.
We support 10 regions and every region comes loaded with default parameters.
These parameters can be changed by the Mac layer or explicitely by the stack
controller layer using APIs provided. This layer in essence detaches Mac completely
from PHY and provides more modular approach to the entire system.
Apart from class structure, the internal functionality is directly deduced from
semtech reference implementation that's why most of the internal data structures are
used on 'as is' basis.
In addition to that, the PHY layer provides APIs to control the LoRaRadio layer, i.e.,
the lora radio driver, ensuring that the radio is accessed from a single entry point.
A seperate data structure file is added which is common to PHY layers only.

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:LORAWAN_FEATURE_BRANCH branch Nov 29, 2017

Adding MAC layer for LoRaWAN implementation
The actual mac algorithms are being used as it is in the reference
implementation.

We introduce an internal class that starts a thread
to handle deffered calls from interrupt context for RTOS. The code base is
compatible with Mbed-OS 2 as well.

GetPhyEventHandlers() API provides mac callback funtions for PHY layer,
which are in turn delegated to radio driver from the PHY layer.

LoRaMacInitialization() is augmented with LoRaPHY parameter which let's
the MAC layer know which particular PHY layer is in use.

LoRaMacSetTxTimer() and LoRaMacStopTxTimer() are used when duty cycle is
off for testing purpose or to support custom application timers.

If the duty cycle is off, mac and phy layer work togather to figure
out the next possible transmission time.

LoRaMacCrypto APIs are provided which provide seemless integration of
mbedTLS into mac layer for cryptography. User application is supposed to
provide proper mbedTLS configuration file.

All other APIs are retained as it is.

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:LORAWAN_FEATURE_BRANCH branch Nov 29, 2017

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2017

@0xc0170 I took care of the namespcae pollution.

LoRaWANInterface is the class users will be using for LoRaWAN stack access https://github.com/ARMmbed/mbed-os/pull/5590/files#diff-e347400efdb946f61d2df07c372c3da5

  • LoRaWANInterface class implements LoRaWANBase which is a pure virtual class
  • Application passes the radio driver to LoRaWANInterface whereas the radio driver
    is inherited from LoRaRadio class which is also a pure virtual class.

Everything else is internal.

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:LORAWAN_FEATURE_BRANCH branch Nov 29, 2017

hasnainvirk added some commits Nov 27, 2017

Adding LoRaWANStack class to control MAC and PHY
LoRaWANStack class is our controller layer on top of our
current MAC and PHY layer. It provides services to an implementation
of LoRaWANBase class.

It is a singleton class owing to the fact that the mac layer underneath
is not a class object. Instead, it uses the MAC via setting mib, mlme, mcps
requests and getting responses back from the mac layer using confirmations and
indications.

In essense this class is a special handle for
mac layer underneath which is predominantly reference design based.
In future we may refactor the LoRaMac.cpp code to make it object oriented
and cleaner.

At one end, it binds the application selected radio driver with the PHY layer
and at the other end it provides services to upper layers handling the mac via
well defined APIs.

For proper selection of a PHY layer, user must use Mbed config system.
For this purpose an mbed_lib.json is provided which can be overriden by the
user defined mbed_app.json. By default the EU868 band is selected as a PHY layer.
User must set relevant keys for the selected connection mechanism.
Adding LoRaWANInterface - implementing, LoRaWANBase
This class is the doorway for the user application into the
Mbed-OS implementation of LoRaWAN protocol. It implements LoRaWANBase
and hence would work with any stack implementation underneath, ensuring
seemless portability for applications.

It takes a pre-constructed object of LoRaRadio and delegates it in the
downward direction. Before calling connect() user must call initialize() function
in order to initialize stack and mac layers.

connect() APIs can be used to either provide relevent keys and connection method at
runtime or compile time (using Mbed config system).

enable_adaptive_datarate() and disable_adaptive_datarate() are used to turn on/off
automatic rate control. Otherwisem set_datarate() could be used to set a particular
data rate on the current channel.

set_confirmed_msg_retries() is valid only for CONFIRMED messages. It means that the stack will
retry for a given number of times before timing out.

set_channel_plan() and get_channel_plan() are used to set or get a particular channel plan.
These APIs are particularly useful in case of ABP (activation by personalization). Because
in case of OTAA(over the air activation), by default the stack takes in a CF List (carrier frequency list)
sent by the base station in conjunction with Network server. This list overwrites all user configured
channels or channel plan. set_channel_plan() can be used to set a single channel as well by setting the
parameter for number of channels to 1.

remove_channel_plan() or remove_channel() are used to remove a currently active channel plan or a specific
channel.

send() and receive() APIs follow posix design except the socket descriptor is replaced with port number here.

lora_event_callback() API is used to set a callback function from the application side which is used by the stack
to inform user of particular events like CONNECTED, DISCONNECTED, CRYPTO_FAILURE, TX_TIMEOUT etc.

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:LORAWAN_FEATURE_BRANCH branch to c743109 Nov 29, 2017

@SenRamakri
Copy link
Collaborator

left a comment

Since this is a large change set is there any design documentation available which makes the review process easier.

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2017

@SenRamakri I can send you the document by email.

@adbridge

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

@hasnainvirk As this is only pre-release code this should NOT be landing on master. This should be going to a feature branch...

@@ -0,0 +1,177 @@
/**
* @file

This comment has been minimized.

Copy link
@SenRamakri

SenRamakri Nov 29, 2017

Collaborator

Is there any design docs available for this large change set. That can help with code reviewing?

* User application data buffer size if compliance test is used
*/
#if (MBED_CONF_LORA_PHY == 0 || MBED_CONF_LORA_PHY == 4 || MBED_CONF_LORA_PHY == 6 || MBED_CONF_LORA_PHY == 7)
#define LORAWAN_COMPLIANCE_TEST_DATA_SIZE 16

This comment has been minimized.

Copy link
@SenRamakri

SenRamakri Nov 29, 2017

Collaborator

I see the phy defines from 0-9 above, the checks here seem to be missing 3,5. Are they not supported? Is this expected?

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Dec 1, 2017

Author Contributor

This is for compliance testing only. We know for sure only about EU868 MHz band. Chinese 470 MHz band is not up yet although Semtech have defined regional parameters for it. Same is the case for some other regions too. I am inclined to change this for only EU868 MHz band. But then I am not sure, how would we certify our stack for other regions. For more information https://github.com/Lora-net/LoRaMac-node check the table LoRaWAN certification results.

****************************************************************************/
bool LoRaWANStack::is_port_valid(uint8_t port)
{
//Application should not use reserved and illegal port numbers.

This comment has been minimized.

Copy link
@SenRamakri

SenRamakri Nov 29, 2017

Collaborator

Can we #define these numbers(224)? Makes it more readable.

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Dec 1, 2017

Author Contributor

These reserved ports don't have names as per specification. That's why they are not defined as a macro.

int16_t LoRaWANStack::prepare_special_tx_frame(uint8_t port)
{
switch (port) {
case 224:

This comment has been minimized.

Copy link
@SenRamakri

SenRamakri Nov 29, 2017

Collaborator

Why do we need switch/case construct to handle just one case?

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Dec 1, 2017

Author Contributor

This part of the code base is incomplete as the Compliance Testing has not started yet. This method is part of Compliance Testing.

}
break;
default:
break;

This comment has been minimized.

Copy link
@SenRamakri

SenRamakri Nov 29, 2017

Collaborator

Is it ok to not set f_buffer_isze when we hit this "default" case, as the function is returning this vale?

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Dec 1, 2017

Author Contributor

As in the previous comment, this part of code base is incomplete. We may need to handle port 0 as well, another reserved part. Anyway, can't really say much as we have not yet started compliance testing.

}

bool LoRaPHYAS923::get_next_ADR(AdrNextParams_t* adrNext, int8_t* drOut,
int8_t* txPowOut, uint32_t* adrAckCounter)

This comment has been minimized.

Copy link
@SenRamakri

SenRamakri Nov 29, 2017

Collaborator

Check for adrNex for NULL before using?

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Dec 1, 2017

Author Contributor

Mac layer makes sure that nothing null is being sent.

}

bool LoRaPHYAS923::rx_config(RxConfigParams_t* rxConfig, int8_t* datarate)
{

This comment has been minimized.

Copy link
@SenRamakri

SenRamakri Nov 29, 2017

Collaborator

Again check for rxConfg == NULL?

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Dec 1, 2017

Author Contributor

Same as above. Mac layer takes care of it

}
}

*drOut = datarate;

This comment has been minimized.

Copy link
@SenRamakri

SenRamakri Nov 29, 2017

Collaborator

Check for drOut == NULL and txPowOut == NULL before using?

uint8_t LoRaPHYEU868::link_ADR_request(LinkAdrReqParams_t* linkAdrReq,
int8_t* drOut, int8_t* txPowOut,
uint8_t* nbRepOut, uint8_t* nbBytesParsed)
{

This comment has been minimized.

Copy link
@SenRamakri

SenRamakri Nov 29, 2017

Collaborator

Check for validity of pointers - linkAdrReq, drOut, txPowOut etc?

lorawan_connect_t connection_params;

if (OVER_THE_AIR_ACTIVATION != 0) {
static uint8_t dev_eui[] = LORAWAN_DEVICE_EUI;

This comment has been minimized.

Copy link
@SenRamakri

SenRamakri Nov 29, 2017

Collaborator

Should this connect routine be made thread safe in case multiple threads call the function simultaneously?

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Dec 1, 2017

Author Contributor

At this moment the system underneath LoRaWANInterface is NonCopyable. It's documented above the constructor of LoRaWANInterface. The intention is to use one and only object of the stack from whichever context it's being accessed

@sg-
Copy link
Member

left a comment

Looking good. Some changes needed though.

Also, I'm not convinced that the attach (callback API) is right. Having the developer manage the state machine does seem very nice. I would thinking about a pattern where we store Callback pointers for each event type but register with a flag. I think this needs some application/user testing to get right.


#include "lorawan/LoRaWANInterface.h"

inline LoRaWANStack& stk_obj()

This comment has been minimized.

Copy link
@sg-

sg- Nov 30, 2017

Member

Return should be a pointer, not a reference as NetworkStack does.

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Dec 1, 2017

Author Contributor

The reference makes sure that we have only one instance and it cannot be NULL.
For consistency it would make sense otherwise I don't see a difference. I wanted to avoid Null checks, if its a reference I can make it const and ensure that it is being passed around correctly otherwise throw me a compilation error. With pointer that is not possible.

static uint8_t nwk_skey[] = LORAWAN_NWKSKEY;
static uint8_t app_skey[] = LORAWAN_APPSKEY;
static uint32_t dev_addr = LORAWAN_DEVICE_ADDRESS;
static uint32_t nwk_id = (LORAWAN_DEVICE_ADDRESS & LORAWAN_NETWORK_ID_MASK);

This comment has been minimized.

Copy link
@sg-

sg- Nov 30, 2017

Member

Is this only used in one place? If so can this be moved from the header file into local scope?

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Dec 1, 2017

Author Contributor

Yeah you are right. I will look into it.

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Dec 5, 2017

Author Contributor

Actually the macros are used elsewhere. And there are other macros as well which are configurable via json and for consistency they should remain together. If I move these ones out, its not consistent anymore. All configurable parameters are defined in lora_mac_data_structures.h

@@ -0,0 +1,402 @@
/**

This comment has been minimized.

Copy link
@sg-

sg- Nov 30, 2017

Member

This file should be alongside other lora code, not netsocket.

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Dec 1, 2017

Author Contributor

This is a pure virtual class just like LoRaWANBase class. It can be used even if the feature lorawan is not available. If we move it to lorawan/ directory it would lose its purpose.

#define LORARADIO_H_

// Radio wake-up time from sleep - unit ms.
#define RADIO_WAKEUP_TIME 1

This comment has been minimized.

Copy link
@sg-

sg- Nov 30, 2017

Member

Who uses this and why not part of the local phy scope?

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Dec 1, 2017

Author Contributor

This is a left over, I will clean it.

This comment has been minimized.

Copy link
@hasnainvirk

hasnainvirk Dec 5, 2017

Author Contributor

Okay, I studied it a bit more. What I find is that this time is used for calculating rx windows parameters and is used by mac and all phy layers. Could move it to lorawan_data_structures though

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

commented Nov 30, 2017

Closing the PR, as it will go to the feature branch.

@sg- sg- removed the needs: work label Nov 30, 2017

@0xc0170

This comment has been minimized.

Copy link
Member

commented Nov 30, 2017

Closing the PR, as it will go to the feature branch.

If the branch is not created, let me know, I'll set up one if needed

@0xc0170 0xc0170 changed the base branch from master to feature-lorawan Nov 30, 2017

@0xc0170 0xc0170 referenced this pull request Nov 30, 2017

Closed

Lorawan feature branch #5620

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2017

'Reopening and Comment' button is disabled for me. @0xc0170

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2017

@AnttiKauppila I can't reopen this PR. And the one #5620 is closed by Martin.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Dec 1, 2017

Github disallows to reopen once it was forced pushed after closing. This is what I found:

We're blocking the pull request reopen if the current head isn't a descendant of the stored head sha (which is what the head was when the pull request was closed). We are not allowing the reopen in that case, because there is no good way to tell what changes have happened while a pull request was closed and the head branch has changed.

I would assume based on this if you can get that branch to the state (SHA) that it was, reopen and then push whatever you have now, it should work :-) If this is too cumbersome, let's reopen the new one.

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2017

@0xc0170 Unfortunately, I already squashed changes based on the comments here in the branch and now my SHAs are totally different. I think we should reopen #5620. I will try my best to entertain all the comments provided here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.