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

open_esplibs: add a skeleton for code in more libraries. #266

Merged
merged 1 commit into from Nov 12, 2016

Conversation

ourairquality
Copy link
Contributor

Have a backlog of a small amount of sdk code converted to source code, basically code needed to support lwip v2, and it might help submit this incrementally if a skeleton could be added for the other libraries and some of their files first. This is still using a linker hack.

@ourairquality
Copy link
Contributor Author

ourairquality commented Nov 11, 2016

Submitted about all that can be for now to here until the skeleton and definitions land. The follow up commits can be seen at https://github.com/ourairquality/esp-open-rtos/commits/ourairquality

This lot gives us code for the paths allocating the struct netif, so the size can be changed.

It also gives us code for most of the paths accessing the netif->flags. Only one read appears to remain in wl_cnx.o:sdk_cnx_sta_leave which checks the NETIF_FLAG_DHCP flag. The NETIF_FLAG_DHCP has been removed in lwip v2, so some lwip hacks are needed to handle this for now until wl_cnx.o is converted to source code too which looks like a big job.

Some uses of netif->hwaddr and netif->state also have code now, but for now it seems more practical to just arrange for their offset to not change. The state is accessed in ieee80211_output_pbuf and perhaps ieee80211_mgmt_output and if there are not too many more references then perhaps it would be practical in time to get code for them all.

The commits are ordered from simple and hopefully safer conversions to the biggest and most likely to have problems namely the hostap code which had a lot of static definitions so required code for many functions. But it seems to work fine with the wificfg example in station and softap modes, and moving the netif slots showed no other writes into netif, and a quick attempt at lwip v2 gave working code too.

@sheinz
Copy link
Contributor

sheinz commented Nov 11, 2016

Hi @ourairquality,

I'm really glad that work on open sourcing binary libraries continues! I will help as much as possible with it.

The hack with linking libraries can be removed if link libraries as a whole archives.
Adding those lines in component.mk makes the linker replace weak symbols in binary libraries with open-source ones:

INCLUDE_SRC_IN_AR = 0

...

WHOLE_ARCHIVES = $(open_esplibs_libnet80211_AR)
WHOLE_ARCHIVES += $(open_esplibs_libphy_AR)
WHOLE_ARCHIVES += $(open_esplibs_libwpa_AR)

EXTRA_LDFLAGS += -Wl,--whole-archive $(WHOLE_ARCHIVES) -Wl,--no-whole-archive

I tried it and it works. I'm not sure if this approach will not cause some other issues.

@ourairquality ourairquality force-pushed the open_esplibs_all_dirs branch 2 times, most recently from ed4558d to 2b0d67c Compare November 12, 2016 06:38
@ourairquality
Copy link
Contributor Author

@sheinz Thank you for the suggestion. The PR now includes a suggestion to use -Wl,--whole-archive adding a per-component WHOLE_ARCHIVES flag which is now also used to omit the source code in only these archives.

#endif /* OPEN_LIBNET80211_ETS */

/* Needed to have the linker actually replace the above definitions. */
void libnet80211_ets_linker_hack(){}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you missed to remove those _hack functions?

@ourairquality
Copy link
Contributor Author

@sheinz Yes, forgot to remove those, fixed thanks.

@sheinz
Copy link
Contributor

sheinz commented Nov 12, 2016

Everything looks good to me except test build is failing due to source files inclusion in an archive.

To fit it in a new approach I would suggest building all test cases as a library with 'WHOLE_ARCHIVE' option set. And link them to the main program.

As the tests are not built by Travis currently. If you don't feel like fixing them within this PR it's ok to do it as a separate PR.

@ourairquality
Copy link
Contributor Author

@sheinz Fixed the building of the tests, thanks.

@sheinz sheinz merged commit bc50c7c into SuperHouse:master Nov 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants