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

net/fib: added configuration header for the FIB #3022

Closed

Conversation

BytesGalore
Copy link
Member

Rationale: this introduces a new header to easily configure the FIB.
Mainly to provide one configuration file for all important defines, i.e. sizes for the actual data fields for FIB entries and universal addresses, and message types.

@BytesGalore BytesGalore added Area: network Area: Networking Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels May 18, 2015
@Lotterleben
Copy link
Member

I think I was one of the people who were pro having this header, but thinking about it again, I was wondering if it would make sense to make some of the fields configurable through functions, more specifically UNIVERSAL_ADDRESS_MAX_ENTRIES, FIB_MAX_FIB_TABLE_ENTRIES, and FIB_MAX_REGISTERED_RP. For example, if you're certain your application only runs one routing protocol, having FIB_MAX_REGISTERED_RP set to 5 would waste valuable space, but if you wanted to change this value, you'd always have to have a modified RIOT branch and keep that up to date... Which can be pretty annoying. Alternatively to the setting (and getting?) by functions thing, a flag-based approach (so that you can set the values in the Makefile) like in https://github.com/RIOT-OS/RIOT/blob/master/pkg/microcoap/0004-increment_max_segments.patch could be an option..
Sorry for thinking of this only just now :(
What do you think?

@BytesGalore
Copy link
Member Author

@Lotterleben good point. I like the idea to configure stuff on compile time like
make FIB_MAX_REGISTERED_RP=1 and so on :)
I will change that tomorrow accordingly

@Lotterleben
Copy link
Member

thanks :)

@BytesGalore BytesGalore force-pushed the fib_add_configuration_header branch 4 times, most recently from 5bebb6f to a3b4e08 Compare June 15, 2015 13:24
@BytesGalore
Copy link
Member Author

@Lotterleben changed to configure on compile time, but this may produce unwanted side-effects.
The defines are set right for the FIB/Universal address only (bad news).

When including the ng_fib.h in a main, for instance in the hello-world example (with USEMODULE += fib of course) and calling say make UNIVERSAL_ADDRESS_SIZE=17,
the main will use the UNIVERSAL_ADDRESS_SIZE default value of 16, while the FIB will initialize and use it with the new value 17. (same applies for all other defines)

You can reproduce it by e.g. printing the value of the defines in the FIB (e.g. in fib_init()) and in the main accordingly.
So I recommend to not provide this compile time configuration.

@Lotterleben
Copy link
Member

Hrrrrm thats not good :( If this is the case, it shouldn't be used, but there must be a way... I'll think about it some more on my way to the Uni.

@Lotterleben
Copy link
Member

Okay, I've opened an issue to document this: #3256
Let's stick to the other way for now and update as soon as the issue is resolved.

@BytesGalore BytesGalore force-pushed the fib_add_configuration_header branch from a3b4e08 to 8df3a4a Compare June 24, 2015 15:11
@BytesGalore
Copy link
Member Author

@Lotterleben agreed, removed the command line configuration possibility.

@Lotterleben
Copy link
Member

thanks, then.. ack if travis says yes

@BytesGalore BytesGalore added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 24, 2015
@jnohlgard
Copy link
Member

WARNING: The following modified files generate doxygen warnings:

sys/include/net/ng_fib.h

ERROR: The following new files generate doxygen warnings:

sys/include/net/ng_fib/ng_fib_config.h`

@BytesGalore BytesGalore force-pushed the fib_add_configuration_header branch 2 times, most recently from 228f294 to 2e89275 Compare July 20, 2015 06:44
@BytesGalore BytesGalore force-pushed the fib_add_configuration_header branch from 2e89275 to 3914716 Compare July 27, 2015 12:02
@cgundogan
Copy link
Member

needs rebase

@Lotterleben
Copy link
Member

Just wondering, do we still want to pursue this one? :)

@BytesGalore
Copy link
Member Author

@Lotterleben I still want to do the housekeeping for the cluttered FIB configuration properties :).
I close this for now and make a new PR to adopt it for the current state and incoming changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants