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

Merge esphome-core into esphome #97

Closed
OttoWinter opened this issue Mar 6, 2019 · 4 comments
Closed

Merge esphome-core into esphome #97

OttoWinter opened this issue Mar 6, 2019 · 4 comments

Comments

@OttoWinter
Copy link
Member

OttoWinter commented Mar 6, 2019

Merge all C++ files into the esphome repository so that the python and C++ files live right next to each other.

This is only an early idea I had the other day, and I haven't thought it through 100%. Just wanted to write it down somewhere and potentially receive some feedback.

The Problem(s)

There are a number of problems in esphome that could all be solved by a major refactor:

  • Compile Time: It can take quite a while to compile a simple esphome project, and each time the user adds a component the whole project needs to be rebuilt. This will only get worse over time with new components.

  • Contributing: Contributing to ESPHome is not that nice, PRs need to be made against 3 repos: esphome, esphome-core and esphome-docs.

  • Viewing Source: Often you want to view the C++ source together with the python source, this is very difficult atm.

  • Custom Components: Creating true custom components (that have python c++ generators too) is not possible. So if someone develops a custom component that doesn't fit into esphome officially (licensing issues, etc) you need to maintain a fork

  • Core and ESPHome versions are out of sync: This bugs me a lot. a) it breaks everyone who attempts to use esphome_core_version: dev and b) when submitting a PR, the python test cases will fail until the core PR has been merged.

  • Defaults are all over the place: Default values for integration parameters are all over the place in different places: some in the integration header file, some in application.h, and some in python validation.

The proposal

Merge esphome-core into esphome

What does that mean? All C++ files will be moved into the esphome repository. I think an example explains this best:
This would be the directory layout for the esphome repo:

esphome
├── core
│   # Core files used by all components; to glue everything together
│   ├── component.cpp
│   ├── component.h
│   ├── esphal.h
│   └── helpers.h
├── components
│   ├── dht
│   │   # Each integration has its own directory
│   │   ├── dht_component.cpp
│   │   ├── dht_component.h
│   │   # This will be called by `sensor: - platform: dht` (see the great HA migration)
│   │   ├── sensor.py
│   │   # Empty file to mark this as a python module (until py3)
│   │   └── __init__.py
│   └── sensor
│       ├── __init__.py
│       ├── filter.cpp
│       ├── filter.h
│       ├── sensor.cpp
│       └── sensor.h
├── __init__.py
├── __main__.py
├── cpp_generator.py
└── dashboard
setup.py
platformio.ini
.travis.yml

As you can see, C++ and python source files are in the same repository, and all source files for a component are
in one directory.

Sample dht_component.h:

#ifndef ESPHOME_DHT_COMPONENT_H
#define ESPHOME_DHT_COMPONENT_H

// note: no more #ifdef USE_DHT_SENSOR

#include "esphome/components/sensor/sensor.h"

namespace esphome {

// note: no more 'namespace sensor {'
class DHTComponent : public PollingComponent {
  // ...
};

}  // namespace esphome

#endif // ESPHOME_DHT_COMPONENT_H

Sample dht_component.cpp:

// note: no more #ifdef USE_DHT_SENSOR
#include "esphome/components/dht/dht_component.h"
#include "esphome/core/log.h"
#include "esphome/core/helpers.h"

namespace esphome {

static const char *TAG = "sensor.dht";

// ...

}  // namespace esphome

Sample dht/sensor.py

DHTComponent = esphome_ns.class_('DHTComponent', PollingComponent)
PLATFORM_SCHEMA = sensor.PLATFORM_SCHEMA.extend({...}))

def to_code(config):
    for pin in gpio_output_pin_expression(config[CONF_PIN]):
        yield
    # Note: No more App.make_dht_sensor
    rhs = DHTComponent.new(...)
    # ...

# Note: No more BUILD_FLAGS
# source files to copy, relative to python file, we could potentially also automate this
SOURCE_FILES = ['dht_component.h', 'dht_component.cpp']

On build, all source files are copied into the src folder.

Improvements:

This will have a number of improvements:

  • Compile Time:

    • We will not have to update global compiler flags when a component is added/removed, we only need to copy/remove files. This means we don't have to recompile.
    • If you don't use component X, it also won't be compiled. Previously the .cpp file was compiled- it was not in the binary because of the #ifdef USE_x but contributed to the compile time.
  • Contributing: Only one PR needs to be made for code changes. (another one is still needed for docs - i won't merge the two though because esphome-docs is quite big with all image assets)

  • Viewing Source: As each component has its own dir, all sources are directly accessible.

  • Custom Components: Would be possible, with the exact mechanism like HA

  • Core and ESPHome out of sync: Obviously no issue anymore

  • Inter-Component Dependencies: This will additionally enforce the barrier between components. Each component will only be able to use core/ code or ones that are explicitly imported in the python file.

Changes Needed

  • All central references to components need to be removed. A component should be 100% self-contained in the directory it is in. The Application App instance is a problem here. So application (which is only a tiny setup helper) will be replaced by direct calls to the component constructor (see dht example above).

  • Some components like esp32_ble_tracker have platforms that do not share the name of the component: xiaomi_mijia, xiaomi_miflora, esp32_rssi etc. This breaks the directory structure a bit - probably the workaround will be a manual override in the component loader (ugly but works).

  • I want to drop all namespaces in this project except for esphome. For example, users had to type cover::COVER_OPEN before, now that would only be COVER_OPEN. namespaces are good for avoiding name conflicts, but I don't think I've ever encountered one yet except for sensor/binary_sensor filters (which are easily renamed).

  • Probably more, will update this post.

Potential Problems

  • Build System: This will only work if it works well with IDEs. Platformio needs to be able to treat the esphome folder as a C++ project. If I don't get platformio to work well together with this new directory structure I will not do this. I need smart syntax highliting and actions in C++ :D
  • Licensing: The ESPHome-Core is licensed under GPLv3, the python code under MIT. Either we relicense everything or maintain the old one with a split license (currently tending to the latter)

This would obviously be a huge refactor, but I think it would be very useful and be an important milestone to make contributing easier.

@glmnet
Copy link
Member

glmnet commented Mar 25, 2019

Interesting. Please avoid use of symbolic links 👍

@B-Kramer
Copy link

I think this would improve the ux greatly, especially for those poor souls compiling on a SOC. It may help you going forward too, as adding a new component would require only a single code change, it may make it easier for you to get some help :)

@mtl010957
Copy link

I like the idea, I agree it would help make the development process a bit easier.

OttoWinter added a commit to esphome/esphome that referenced this issue Apr 17, 2019
## Description:

Move esphome-core codebase into esphome (and a bunch of other refactors). See esphome/feature-requests#97

Yes this is a shit ton of work and no there's no way to automate it :( But it will be worth it 👍

Progress:
- Core support (file copy etc): 80%
- Base Abstractions (light, switch): ~50%
- Integrations: ~10%
- Working? Yes, (but only with ported components).

Other refactors:
- Moves all codegen related stuff into a single class: `esphome.codegen` (imported as `cg`)
- Rework coroutine syntax
- Move from `component/platform.py` to `domain/component.py` structure as with HA
- Move all defaults out of C++ and into config validation.
- Remove `make_...` helpers from Application class. Reason: Merge conflicts with every single new integration.
- Pointer Variables are stored globally instead of locally in setup(). Reason: stack size limit.

Future work:
- Rework const.py - Move all `CONF_...` into a conf class (usage `conf.UPDATE_INTERVAL` vs `CONF_UPDATE_INTERVAL`). Reason: Less convoluted import block
- Enable loading from `custom_components` folder.

**Related issue (if applicable):** esphome/feature-requests#97

**Pull request in [esphome-docs](https://github.com/esphome/esphome-docs) with documentation (if applicable):** esphome/esphome-docs#<esphome-docs PR number goes here>

## Checklist:
  - [ ] The code change is tested and works locally.
  - [ ] Tests have been added to verify that the new code works (under `tests/` folder).

If user exposed functionality or configuration variables are added/changed:
  - [ ] Documentation added/updated in [esphomedocs](https://github.com/OttoWinter/esphomedocs).
@OttoWinter
Copy link
Member Author

Done. Unfortunately, all existing PRs are invalid because of this, but I think the improved API/development process will be worth it.

@esphome esphome locked and limited conversation to collaborators Jun 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants