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

LoRaWAN: Retiring LoRaWANBase class #9219

Merged
merged 4 commits into from Feb 21, 2019

Conversation

@hasnainvirk
Copy link
Contributor

commented Jan 1, 2019

Description

It was decided to retire LoRaWANBase class which served
as a pure virtual interface class from which LoRaWAN network stack
implementations would get inherited. However, the current view is that
we may be the only user of it so we could retire LoRaWANBase.

Pull request type

[ ] Fix
[X] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@AnttiKauppila @kjbracey-arm

Release Notes

There has been only one user of the LoRaWANBase class, the LoRaWANInterface. We anticipated that the third parties may use this class to provide their own LoRaWAN network stacks alongwith the default network stack provided by Mbed OS. However, a trend has been observed that the default stack provided by Mbed OS has become the de-facto choice. Having LoRaWANBase class adds 1K to the ROM in terms of footprint (for GCC, ARMCC and IAR optimize that already). It has been decided to do away with the LoRaWANBase class for now, and if the need arises it can be reintroduced back.

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Jan 1, 2019

@ciarmcom ciarmcom requested review from AnttiKauppila, kjbracey-arm and ARMmbed/mbed-os-maintainers Jan 1, 2019

@mattbrown015

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2019

Hi @hasnainvirk,

I like this PR! :-)

In our build we modify LoRaWANInterface so that it no longer inherits from LoRaWANBase.

But, crucially, we also removed all the 'virtual' keywords from the member functions.

Making the member functions non-virtual reduced our executable size by over a 1000 bytes. 1000 bytes is a significant saving. My feeling is that making the functions non-virtual meant the linker could see that they weren't used and remove them from the executable. Obviously other people's mileage would vary.

Do you believe that LoRaWANInterface needs to be polymorphic?

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

commented Jan 3, 2019

@mattbrown015 Unfortunately we need to pay the price of 1000 bytes here because we wish to leave a window for application developers to write a single application that can be used with any LoRaWAN network stack. Actually that was the sole purpose of the dear departed LoRaWANBase :).
Now that she's gone, we still need the APIs in LoRaWANInterface to be overridable. I wouldn't mind having a 1000 bytes back, but this is a thing which we need to discuss internally first as it may not be entirely in line with Mbed philosophy.

@mattbrown015

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2019

It's fine by me if the functions remain virtual. Our build process has a mechanism for applying patches to make changes that we want and while not perfect it suits our needs.

But, I'm interested because I'm interested in C++ and API design! :-)

Is making the class run-time polymorphic the only way to make it flexible?

If an alternative implementation is required is it not possible to just override non-virtual functions in a new derived class?

Virtual functions are only necessary if the app needs to pass around base class pointers without caring which stack implementation is being used. Or to put it another way, do you expect clients to select the stack implementation at run-time.

If we were working on server software running on x64s we would probably put on our best C++ hats and program everything to interfaces. But we're embedded so we probably want to leverage compile-time techniques.

(Feel free not to reply, both of us should probably be concentrating on real work!)

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Jan 3, 2019

But, I'm interested because I'm interested in C++ and API design! :-)

Ping @kjbracey-arm @pan- ;)

@mattbrown015

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2019

I thought of another way of looking at it!

The phy class is polymorphic.

Currently we statically construct a EU868 phy and pass it to LoRaWAN as a base class pointer. In this case the polymorphism is an overhead that we're not using. But, I can see that a mobile device might need to change its phy at run-time as it moves RF jurisdictions. Or, perhaps more likely, production is simplified by building all devices to the same spec and then configuring them for the intended jurisdiction when they are deployed.

Comparing the phy to stack implementation. The API currently adds overhead for all your customers on the basis that you think you might have a few customers that want to select the stack implementation at run-time.

It's your API. You need to decide if the price paid by all your customers is worth it to make life easier for a few.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2019

@mattbrown015 I'm curious - was your 1000 byte figure with GCC or one of the other toolchains? GCC is particularly disadvantaged here, as it can't do unused virtual function elimination, unlike IAR and ARM C.

We certainly want/need the polymorphism for the cases where we are passing around stuff where there are multiple viable implementations NetworkInterface and Socket in particular.

I would like to regard optimising it as a tools issue - they should be capable of eliminating unused virtual stuff generally, and with Link-Time Optimisation, it should even be possible to totally eliminate the virtuals if the compiler+linker can see that only one derived type is instantiated.

Maybe I'm unduly optimistic though.

@mattbrown015

This comment has been minimized.

Copy link
Contributor

commented Jan 7, 2019

Hi @kjbracey-arm, yes we're using ARM GCC 7. I've just noticed that ARM GCC 8 was released in December; that means Mbed OS is even further behind in its support for ARM GCC 6. ;-)

It's interesting that this is an example of where the more expensive compilers add some value. We won't be changing compiler for our project though, in theory we're nearly finished!

If you have concrete examples that justify your design that's fair enough, like I say, it's your API! :-)

@pan-

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

Maybe I'm unduly optimistic though.

I think it would be interesting to do some tests with Link Time Optimization (LTO) enabled to identify and document the pattern that can be optimised and those that can't. The issue lies in making sure that the linker optimise the application as a whole and do not consider dynamic link which prevent vtable deletion.

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

@kjbracey-arm @pan- @AnttiKauppila I suggest that we do away with virtual modifier in LoRaWANInterface as it brings little value after retiring LoRaWANBase.
If not, then we shouldn't retire LoRaWANBase at all. Because if a third party wishes to provide an implementation of their stack it would make sense to inherit from LoRaWANBase. LoRaWANInterface have embedded objects of LoRaWANStack class and other system level classes which the third party may not require.

@AnttiKauppila

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

@hasnainvirk You are correct. LoRaWANStack is a bit problematic currently. But only because bind_phy_and_radio_driver() is called in the constructor (Cannot be overwritten). We need to think more about this. If we init the radio in constructor and call bind later, it should work.

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:lorawanbase_migration branch Jan 9, 2019

@cmonr cmonr added needs: work and removed needs: review labels Jan 9, 2019

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

@AnttiKauppila In d05e7be we have done away with virtual modifier. In the near future we can go forward for more refactoring involving LoRaWANStack, maybe like combining LoRaWANInterface and LoRaWANStack.

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:lorawanbase_migration branch Jan 10, 2019

@AnttiKauppila
Copy link
Contributor

left a comment

I want to have a blessing from architects to get rid of virtual methods. It has been Mbed OS design to be able to override connectivity stacks

@hasnainvirk hasnainvirk force-pushed the hasnainvirk:lorawanbase_migration branch Jan 10, 2019

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

@AnttiKauppila Totally agree. We need to know if that is alright with them. @kjbracey-arm Any comments ?

hasnainvirk added some commits Jan 1, 2019

Retiring LoRaWANBase class
It was decided within the team to retire LoRaWANBase class which served
as a pure virtual interface class from which LoRaWAN network stack
implementations would get inherited. However, the current view is that
we may be the only user of it so we could retire LoRaWANBase.
Deprecation notice for LoRaWANBase
A deprecation notice has been added for any users of LoRaWANBase and any
existing reference is redirected to LoRaWANInterface.
Removing virtual modifier from LoRaWANInterface
Doing away with virtual modifier from LoRaWANInterface, gets rid of
vtable for LoRaWANInterface.
Doxygen corrections
Adding group identidier so that LoRaWANInterface class goes to the class
hierarchy section rather than data-structures.

Adding missing documentation for a couple of public functions.

Adding \code and \endcode modifiers for the example code in the
documentation.

Adding compile time NO_DOXYGEN flag for the implementations of the
LoRaPHY Class.

Adding documentation for some of the private structures.
@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

@donatieng please review (for @bulislaw)

@cmonr cmonr added needs: review and removed needs: work labels Feb 11, 2019

@hasnainvirk

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

@0xc0170 Can we please prioritize that for CI and merge ?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

@0xc0170 Can we please prioritize that for CI and merge ?

I would like to have this reviewed by @donatieng or @bulislaw . IN the meantime we can start CI

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

CI started

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Feb 14, 2019

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 14, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 1
Build artifacts

@cmonr cmonr added needs: review and removed needs: CI labels Feb 15, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2019

Waiting on ok from @donatieng

@donatieng

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

@ARMmbed/mbed-os-maintainers this showcases how each team should be explicit about which APIs are:

  • User-facing (public API)
  • Partner-facing (porting layer)
  • Internal

In this case this PR maintains compatibility for users (using the typedef), however it is very much a breaking change for partners (even if no one is using it - but are we sure?). In that regard the TAM team (@screamerbg @MarceloSalazar @ashok-rao) should give their OK before this gets merged.

And we should make sure all teams clearly define what their porting layers are, but that's a much wider topic :).

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 15, 2019

In that regard the TAM team (@screamerbg @MarceloSalazar @ashok-rao) should give their OK before this gets merged.

Please review

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

@ashok-rao Based on the recent discussions, this is good to go?

@ashok-rao
Copy link
Contributor

left a comment

Approved based on email discussions with Etteplan. Thanks!

@0xc0170 0xc0170 merged commit d030c04 into ARMmbed:master Feb 21, 2019

22 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10416 cycles (+782 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
@AnotherButler

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

Could someone please update the content on our porting guide: https://github.com/ARMmbed/mbed-os-5-docs/blob/development/docs/porting/connectivity/LoRaPortingGuide.md

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

@AnttiKauppila

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

@hasnainvirk Please update the documentation Amanda is referring.

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.