Skip to content

Rainmaker library extension #6813

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Jul 6, 2022
Merged

Rainmaker library extension #6813

merged 10 commits into from
Jul 6, 2022

Conversation

sivar2311
Copy link
Contributor

Description of Change

This PR extends the ESP Rainmaker functionality.

  • Added definitions for various parameters and UI types.
  • New overload of the value function with parameter type const char* to prevent a compiler warning with this type.
  • New function addValidStrList added to Param class. This is required for using a Mode parameter.
  • Added new RMakerCustomDeviceAirCooler example to demonstrate the use of Toggle, Mode and Range parameters.
    This is the Arduino version of the example from the chapter Building Custom Devices / 3rd Party Integrations.

Tests scenarios

I have tested my Pull Request on Arduino-esp32 core v2.0.3 with esp32doit-devkit-v1 Board with the new provided RMakerCustomDeviceAirCooler example.

@me-no-dev
Copy link
Member

The changes you have done to the files in tools/sdk/chip/... can not be done in this repository. Files in that folder come from ESP-IDF or one of it's external components (esp-rainmaker in this case). You can either make a pull request to the proper repository or add the definitions in the Arduino rainmaker headers.

@VojtechBartoska VojtechBartoska added the Area: Libraries Issue is related to Library support. label May 27, 2022
@sivar2311
Copy link
Contributor Author

Hi @me-no-dev!
Thanks for the review and feedback.
Yes, I was not sure about these files.

I don't know exactly how the workflow is from the ESP-IDF or Rainmaker repository to the arduino-esp32 repository.
To avoid code duplication and possible errors I think to do a pull request in the Rainmaker repository first.

Can this current pull request stay until the changes are applied to the Rainmaker / ESP-IDF repository, or is there a better way?

@me-no-dev me-no-dev marked this pull request as draft May 27, 2022 15:18
@me-no-dev
Copy link
Member

yup :) I've turned it into draft for now.

@sivar2311
Copy link
Contributor Author

For tracking: PR in espressif/esp-rainmaker#126

@sivar2311 sivar2311 mentioned this pull request Jun 8, 2022
Merged
@sivar2311
Copy link
Contributor Author

Hi @me-no-dev The PR in espressif/esp-rainmaker#126 has been merged.
What are the next steps to bring these changes to the arduino-esp32 repository?

@me-no-dev
Copy link
Member

Hi @sivar2311, thanks for letting me know. I have updated the lib-builder and generated the new libs. They are currently here and will soon be merged into master in preparation for 2.0.4 release. Please revert the changes to the headers in the "sdk" folder of this pull request as those will now come from the component itself through the lib builder. Also please squash the commits. I will run CI and merge as soon as we merge the IDF updates.

@sivar2311
Copy link
Contributor Author

Hi @me-no-dev !
Thanks for the info. I will make the necessary changes as soon as possible.

Off-topic:
The whole construct from ESP-IDF to Arduino ESP32 is still very complex and obscure for me. Are there detailed descriptions or instructions that explain the relationships and processes? I would like to go deeper into the matter and contribute more.

@me-no-dev
Copy link
Member

This repository contains the ESP-IDF project and scripts that build ESP-IDF, collect and copy everything necessary to Arduino: https://github.com/espressif/esp32-arduino-lib-builder

@sivar2311
Copy link
Contributor Author

@me-no-dev Thanks for the information about the esp32-arduino-lib-builder.
I have now reverted the changes to the header files from the "sdk" folder.

@me-no-dev me-no-dev marked this pull request as ready for review June 13, 2022 16:38
@me-no-dev me-no-dev changed the base branch from master to idf-release/v4.4 June 13, 2022 16:38
@me-no-dev me-no-dev changed the base branch from idf-release/v4.4 to master June 13, 2022 16:40
@me-no-dev me-no-dev changed the base branch from master to idf-release/v4.4 June 13, 2022 16:41
me-no-dev
me-no-dev reviewed Jun 13, 2022
View reviewed changes
libraries/RainMaker/src/RMakerParam.h Outdated
@@ -47,5 +47,6 @@ class Param
esp_err_t addUIType(const char *ui_type);
esp_err_t addBounds(param_val_t min, param_val_t max, param_val_t step);
esp_err_t updateAndReport(param_val_t val);
esp_err_t addValidStrList(const char** str_list);
Copy link
Member

Choose a reason for hiding this comment

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

your function declaration does not match the implementation. In the implementation you have two arguments: const char **string_list, uint8_t count. Probably you missed the second one.

@me-no-dev
Copy link
Member

@sivar2311 note that the target branch has changed to idf-release/v4.4, because that is where your changes are :)

@sivar2311
Copy link
Contributor Author

@me-no-dev

your function declaration does not match the implementation. In the implementation you have two arguments: const char **string_list, uint8_t count. Probably you missed the second one.

Sorry for the inconvenience, this is embarrassing.
I saw the error message yesterday but I was already in bed.

I already had the error during development and I had already fixed it - but probably only in my local platform directory (PlatformIO) which is used for the build process. Unfortunately I was not able to get PlattformIO to use my local copy of the Arduino-ESP32 repository for the build process.

I have fixed the bug in #0cb567d

note that the target branch has changed to idf-release/v4.4, because that is where your changes are :)

Okay, that makes sense :) Do I need to change anything on my side?

@me-no-dev
Copy link
Member

Seems you have some other errors: https://github.com/espressif/arduino-esp32/runs/6875362072?check_suite_focus=true#step:5:164

Okay, that makes sense :) Do I need to change anything on my side?

No, you should be ok as-is, just make sure you have updated your fork

@sivar2311
Copy link
Contributor Author

The definition was missing for the ESP32C3 target. This is fixed now in #de53f6f
Next time I will set up a better test suite that compiles and tests for all targets.

Thanks for your patience :)

@me-no-dev
Copy link
Member

@sivar2311 everything looks good now :) will merge before 2.0.4 release

@me-no-dev me-no-dev added the Status: Pending Merge Pull Request is ready to be merged label Jun 14, 2022
@sivar2311
Copy link
Contributor Author

Yeayyy
Thank you very much! Also for the support.
I'll try my best to do better next time :)

@me-no-dev me-no-dev changed the base branch from idf-release/v4.4 to master June 16, 2022 10:41
@me-no-dev me-no-dev merged commit c93bf11 into espressif:master Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@me-no-dev me-no-dev me-no-dev left review comments

Assignees
No one assigned
Labels
Area: Libraries Issue is related to Library support. Status: Pending Merge Pull Request is ready to be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sivar2311 @me-no-dev @VojtechBartoska