search

PelionIoT / nanostack-libservice

Other
2 stars 11 forks source link

Remove ns_trace.h #99

Closed AnttiKauppila closed 3 years ago

AnttiKauppila commented 3 years ago

ns_trace.h was only including mbed-trace.h and therefore causing useless dependency for mbed-trace library. By removing it we remove circular dependency between this and mbed-trace library.

This PR needs to be merged first: PelionIoT/mbed-trace#102

artokin commented 3 years ago

ns_trace.h is included from quite a many components and they needs to be updated first.

mikter commented 3 years ago

@kjbracey-arm Could you help us understand how the porting and library dependencies are meant to be

AnttiKauppila commented 3 years ago

ns_trace.h is included from quite a many components and they needs to be updated first.

Why, are you including master branch only and not the commit hash?

AnttiKauppila commented 3 years ago

I checked the mbed-trace and it is an independent module so it should be fine to use it in any platform. ns_trace is included more or less 1000 time in our organisation so it will be quite a big task to update all of those dependencies but I guess we should just do it

mikter commented 3 years ago

ns_trace.h is the porting file for nanostack so we should always include that not any Mbed OS libraries. When someone is making port for nanostack tracing they should only need to modify the ns_trace.h file to create new tracing system

AnttiKauppila commented 3 years ago

mbed-trace is NOT Mbed OS library, it uses only std libraries, so if someone wants to modify traces, they have a good starting point

mikter commented 3 years ago

ns_trace.h should include any trace library that is used in that environment where software is compiled. The Mbed-trace is copied from ns_trace.h to allow others to use it not the other way around. So ns_trace.h should be as it is and included from nanostack as it is the porting file for nanostack traces.

Some other solution should be made to remove the circular dependency. I dont know what is the correct solution though so I would like to have input from @kjbracey-arm and @artokin what is the best approach.

What is actually causing the circular dependency?

artokin commented 3 years ago

Dissucced with @AnttiKauppila , two mbed_trace functions are causing the problem: mbed_trace_ipv6_prefix and mbed_trace_ipv6. They use ip6_prefix_tos and ip6tos from nanostack-libservice/source/libip6string/ip6tos.c.

kjbracey commented 3 years ago

How's about just moving "ns_trace.h" into "mbed-trace"?

When ns_trace became mbed-trace, we wanted to retain the compatibility, so we left the ns_trace.h in libservice to link to it. But that header could be provided by mbed-trace itself.

Alternatively make ns_trace.h a separate component on its own - it's presumably circular because it's being regarded as part of the "libservice" component. But it isn't really part of libservice - it's just the "porting glue" to provide tracing via mbed-trace. Make it a "libservice-mbed-trace" component. (Compare the separate ns-hal-mbed-cmsis_rtos component - porting layer for other stuff on top of mbed OS + cmsis).

Either of those seem fine. First one (ns_trace.h inside mbed-trace) is easier if it works. Second is a tad cleaner, and conceptually right, but an extra component for one 1-line header feels a bit silly.

AnttiKauppila commented 3 years ago

I vote for moving ns_trace.h to mbed-trace.

mikter commented 3 years ago

I am ok with moving the ns_trace.h file

AnttiKauppila commented 3 years ago

https://github.com/PelionIoT/mbed-trace/pull/102