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

How we deal with the ESP8266 and ESP32 forks within this one repository #1657

Closed
TerryE opened this issue Dec 10, 2016 · 14 comments
Closed

How we deal with the ESP8266 and ESP32 forks within this one repository #1657

TerryE opened this issue Dec 10, 2016 · 14 comments

Comments

@TerryE
Copy link
Collaborator

TerryE commented Dec 10, 2016

Sorry guys, but this issue doesn't fall neatly into the Bugrep ot missing feature categories, but it is closer to the latter.

Key Issue

@jmattsson Johny has adopted a pragmatic view in respect of his ESP32-dev fork and as a consequence the directory and file structure is incompatible with that of master or dev, and therefore this fork can never be merged with either without destroying our ability to build ESP8266 configuration. I feel that this is a step that takes the NodeMCU project in the wrong direction.

As I commented in #1319, whilst we have to accept that there are aspects of these target architectures that mean that we need to accept that there will be esp8266 and esp32 specific aspects that need to be separated into two different code hierarchies, wherever practical for those major subcomponents which are largely common we should maintain a common code base (even if it then contains minor preprocessor variants embedded in the code).

Justification

If we don't do this then in practice the project will split into the two H/W architecture variants and individual contributors will end up focusing on a single architecture and therefore our contributor pool will be divided in two.

Proposal for Discussion

I feel that we should at a minimum attempt to maintain a common code base and unified directory / file structure for the lua, docs, tools, lua_examples subcomponents. We should also have esp8266 and esp32 hierarchies each containing subdirectories such as platform and sdk. This is going to involve a lot of house-cleaning of the existing code base. But the only other option as I see it is to have a clean break and fork the repository into two separate repositories.

At a minimum we should keep the lua codebase common, and in the event that we do decide to split into two separate repositories. then we should establish a separate common imported repository for this common code. I will raise a separate specific issue to cover this and discuss the details. This isn't going to be a quick and easy exercise, but we should attempt to implement this within the next 3-6 months. I suggest that I take the lead on this with the support of @pjsg philip if he is willing. In the meantime I also suggest that we declare a voluntary moratorium on non-essential changes to the lua hierarchy for promotion to dev or master. (See #1661 for more discussion).

@devsaurus
Copy link
Member

I started a trial to merge the esp8266 and esp32 branches over at https://github.com/devsaurus/nodemcu-firmware/tree/test_unified_repo. It's basically plain dev with dev-esp32 merged in (files&folders renamed where necessary) and both platforms appear to build properly.
They don't share much files yet - I just unified tools and lua_examples up to now which were the lowest hanging fruits.

What's ahead of us from my point of view:

  • Source code for esp8266 still lives in app while source for esp32 is in components. Technically it would be simple to have e.g. components_esp8266, components_esp32 and a common components. I'm hesitating here since that would introduce a lot of git noise. Is this cosmetic correction worth the troubles (pending PRs will be invalidated e.g.)?
  • READMEs and docs need to be unified somehow. At the moment readthedocs.org would only build the esp8266 doc tree.
  • Travis CI should be extended to build esp32 as well.
  • esp8266 sdk should be submoduled like for esp32 IMO. Right?
  • Source code... this will be the major benefit. Modules like lua, bit, encoder seem to be the most simple ones to unify. I only found the c_string.h / c_whatever topic to be the core blocker. Somebody with a smart idea to abstract this?

If this setup is considered good enough, I'd go for a PR against dev for detailed review and comments. What do you think?

@TerryE
Copy link
Collaborator Author

TerryE commented Jun 15, 2017

Arnim, I am just about to turn in, so I will keep this short.

I think at this stage we should be exploring options rather than going straight to implementation.

There's some aspects of the Lua 5.3 port that I've reworked twice now as I've got into the implemetation realities. My current thinking on the c_*.h encapsulation -- as far as the Lua core is largely to ignore it and link against the gcc newlib headers, but handle their mapping to the c_* and ets_* entry points through a mix of top level define mappings and ld provide directives. Give a week or so to get my core build working on this basis and I will push as a new evaluation branch for you guys to see what I mean.

@TerryE TerryE closed this as completed Jun 15, 2017
@TerryE TerryE reopened this Jun 15, 2017
@devsaurus
Copy link
Member

I think at this stage we should be exploring options rather than going straight to implementation.

Regard my implementation trial as the outcome of such local exploration, triggered by our discussion on email channel. I have a clearer view on certain topics now.

From your original post:

We should also have esp8266 and esp32 hierarchies each containing subdirectories such as platform and sdk.

Yes, we need these hierarchies also for technical reasons. My first trial was based on a flat approach where all subdirs were contained in one components folder. The ESP-IDF build system got into troubles there when a subdir is present with the same name like a component in the sdk (e.g. the coap for esp8266, even though it was told to skip this subdir).

I also figured that we can name the hierarchies arbitrarily. There's no technical restriction as far as I can see. ESP8266 build system is agnostic in this respect, and ESP32 build system will use any naming spec we provide in its toplevel makefile.

This is going to involve a lot of house-cleaning of the existing code base.

For plain merging in my trial this effort was surprisingly low. Effort for subsequent unification items could be stretched over time if we address them one by one within the live system.

and in the event that we do decide to split into two separate repositories. then we should establish a separate common imported repository for this common code.

I would not recommend this approach. Code in submoduled repositories should be considered mostly invariant (released artifacts like sdk etc.) - I would not want to develop in an hierarchical environment where you need to update a submodule a couple of times for each edit. Well, and the github PRs are not ready to stretch over 2 repos/branches consistently, so I expect headaches with project management as well.

Which are further specific aspects where you see the need to explore options? I'd be glad to bring in my experience.

@TerryE
Copy link
Collaborator Author

TerryE commented Nov 21, 2018

@devsaurus Arnim et al, I think that it is time to dust this Issue off and start moving on it. However, I am also hobbled by the fact that I still have no practical experience with the ESP-IDF, so I will need to lean of you for this. I suggest that we committers should first agree some principles taking into reasonable consideration any other reasoned developer input. So how about the following as a starting suggestion for at least some of them?

  • We should actively aim to minimise difference in the codebase. My reason here is simple: having unnecessary variants just makes more work in keeping the two variants current. Most of the following suggestions really follow from this.
  • We should use a unified directory hierarchy for both builds. Again, it makes no sense having the same file in different locations in the two variants. The ESP32 build system is constrained to conform the ESP IDF rules, but the ESP8266 version is entirely within our control and a couple of us totally understand its build system, so it makes sense in general to restructure the ESP8266 hierarchy to align it the ESP32 structure, even though the ESP8266 make has nothing to do with the IDF.
  • Wherever practical directories should be identical for both branches or unique to a branch. In the case of C code, where we can we should use conditional compilation to handle any variant issues. I can do this for Lua. We need to have a systematic approach for modules and their subordinate libraries.
  • Wherever practical drivers and equivalent H/W related libraries should have a unified API across the two variant so that Lua modules and code that call these can be common to both.

I could go on, but I will defer to others for general feedback first. There are many areas where in my view this leads to a clear unified approach, but there are others such as the driver hierarchy where the structure isn't clear to me. So comments?

@pjsg
Copy link
Member

pjsg commented Nov 21, 2018 via email

@devsaurus
Copy link
Member

@TerryE Thanks for reviving this.
Generally agree to all your points. I added my random thoughts below.

We should actively aim to minimise difference in the codebase

Probably the biggest amount of effort will go into this aspect. But it could be tackled step-by-step based on a roadmap (in contrast to having eliminated all differences at day 1).

We should use a unified directory hierarchy for both builds

Although I don't expect that we're significantly constrained by ESP-IDF. Its build system can be adapted quite freely with project variables.
We might decide to restructure ESP8266 and ESP32 hierarchies to enable coexistence of both platforms, but IMO we're not forced by ESP-IDF requirements to make the ESP8266 build look like a ESP32 project.

Wherever practical directories should be identical for both branches or unique to a branch
We need to have a systematic approach for modules and their subordinate libraries.

I think this is generally feasible, the key point is the "systematic approach". It probably should cover concepts for both "unification" and "separation" on appropriate abstraction levels.

  • Unification e.g. of module C code, which needs to enabled by separation on platform level (i.e. different app/platform/* implementations).
  • Separation of module C code would lead e.g. to different feature sets of the firmware flavors.

Wherever practical drivers and equivalent H/W related libraries should have a unified API across the two variant

I would support this API design goal. However, I see benefits in exploiting specific H/W features of either platform.
Can both goals be covered by an inclusive approach? E.g. a common, basic API set with specialized, platform dependent additions/extensions?

@jmattsson
Copy link
Member

Well, if we can make the ESP8266 version build within the IDF framework (or sufficiently integrate with/within it), then we might even get to the point where have can have a single branch which can build for both chips.

I will however note that while I'm all for having a unified interface, I am firmly against the type of pessimisation which easily happens if trying to adhere to the considerably more limited 8266. I'd rather see a full-featured variant for the 32 then than a "fully compatible" dumbed down version which still isn't actually "fully compatible" due to fundamental hardware differences. It's a fine line to walk, and I'm sure we'll step off the line in both directions at times.

@TerryE
Copy link
Collaborator Author

TerryE commented Nov 24, 2018

@jmattsson Johny, at the moment we have a hobbled ESP32 version, not the other way around. I agree that we shouldn't limit the ESP32 implementation, and that hasn't been suggested, but we should be able to develop Lua applications on one architecture and then move them to the other with minimal work -- if both architectures support the devices we want to use. That means:

  • The Lua engine and VM should be common
  • The Lua APIs for I2C, SPI, OW should be common or at least have a common core
  • Ditto the module API, and the Node and File modules, and any other modules such as sjson, crypt, etc.
  • When it makes sense to implement modules in Lua such as those layered over I2C and the other common bus architectures then we should deprecate C implementations and ensure that such modules work on both architectures.

We have very few active core committers at the moments and we can't afford them split across the two H/W platforms unnecessarily.

@ildar
Copy link

ildar commented Aug 12, 2019 via email

@TerryE
Copy link
Collaborator Author

TerryE commented Aug 13, 2019

@ildar, If you look at the date of my last comment, it was 10 months ago, which is why it wasn't a plan but more my initially raising the issue with the other committers. We've really superseded this with later discussions, so perhaps it would be best to close this stale thread.

@TerryE TerryE closed this as completed Aug 13, 2019
@ildar
Copy link

ildar commented Aug 13, 2019 via email

@TerryE
Copy link
Collaborator Author

TerryE commented Aug 13, 2019

Yup, it is. If you filter the open issues for the recent ones raised by me and review these then you will get a good idea of what we are doing. For example #2838 is tidying our make hierarchy and one aspect of this is that we've removed the hardwired references to app so that we can rename it to components if we wish.

A lot of what we are doing to prepare our Lua 5.3 release is a precursor to the merge, and unfortunately some of the discussions are on a private team thread. Once constraint is the running under RTOS is more RAM hungry then under the non-OS SDK, and our community always wants to have more RAM so we can't move both platforms to RTOS. Nonetheless, we've bee doing a lot of prep work (e.g. #2838) with a view to ultimately using the IDF for both platforms albeit with the ESP8266 makes generating an SDK based image. But at the mo, I am in the middle of Lua53 work, so I will leave it with that.

@ildar
Copy link

ildar commented Aug 19, 2019 via email

@nwf
Copy link
Member

nwf commented Aug 19, 2019

In general, I think our APIs are... up for renewal when appropriate. Feel free to cherry-pick the best from both platforms, but don't feel completely beholden to either.

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

No branches or pull requests

6 participants