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

[ESP] Add ESP32C3 DevKitM support to all-clusters-app #6345

Merged
merged 2 commits into from
Jun 24, 2021

Conversation

wqx6
Copy link
Contributor

@wqx6 wqx6 commented Apr 28, 2021

Problem

Need to add ESP32C3 support to the all-clusters-app.

Summary of Changes

  1. Update the ESP-IDF to release/v4.3 branch, which supports ESP32C3 module.
  2. Add ESP32C3 module support to all-clusters-app.
  3. Fix wrong TLVWriter::Put function The wrong TLVWriter::Put function will be called with a unsigned 8-bits(or 16-bits) value passed in #7784

Testing

  1. Compiling success with all-clusters-app,lock-app, shell, persistent-storage, temperature-measurement-app and pigweed-app for ESP32 with idf-v4.3.
  2. Tested locally with chip-device-ctrl with all-clusters-app for ESP32C3-DevKitM and ESP32-DevKitC

@CLAassistant
Copy link

CLAassistant commented Apr 28, 2021

CLA assistant check
All committers have signed the CLA.

@woody-apple
Copy link
Contributor

@NoyceOne0630 Thanks for the contribution! Can you please sign the CLI, and also restyle the PR per the instructions in:
#6346

examples/all-clusters-app/esp32/README.md Outdated Show resolved Hide resolved
examples/all-clusters-app/esp32/README.md Outdated Show resolved Hide resolved
examples/all-clusters-app/esp32/CMakeLists.txt Outdated Show resolved Hide resolved
examples/all-clusters-app/esp32/Makefile Outdated Show resolved Hide resolved
examples/all-clusters-app/esp32/main/CMakeLists.txt Outdated Show resolved Hide resolved
examples/all-clusters-app/esp32/main/CMakeLists.txt Outdated Show resolved Hide resolved
@wqx6 wqx6 changed the title Add ESP32C3 DevKitM support to all-clusters-app [WIP] Add ESP32C3 DevKitM support to all-clusters-app Apr 29, 2021
@wqx6 wqx6 requested a review from dhrishi April 30, 2021 07:04
@todo
Copy link

todo bot commented Apr 30, 2021

Not supported

// TODO: Not supported
}
exit:
if (err != CHIP_NO_ERROR)
{
ChipLogError(DeviceLayer, "Disabling CHIPoBLE service due to error: %s", ErrorStr(err));
mServiceMode = ConnectivityManager::kCHIPoBLEServiceMode_Disabled;
}
}


This comment was generated by todo based on a TODO comment in 6318fa7 in #6345. cc @wqx6.

@todo
Copy link

todo bot commented Apr 30, 2021

fail connection???

// TODO: fail connection???
}
return;
}
CHIP_ERROR BLEManagerImpl::HandleTXComplete(struct ble_gap_event * gapEvent)
{
ChipLogProgress(DeviceLayer, "Confirm received for CHIPoBLE TX characteristic indication (con %" PRIu16 ") status= %d ",
gapEvent->notify_tx.conn_handle, gapEvent->notify_tx.status);


This comment was generated by todo based on a TODO comment in 6318fa7 in #6345. cc @wqx6.

@wqx6
Copy link
Contributor Author

wqx6 commented May 7, 2021

the riscv legacy __sync APIs will be added to esp-idf release/v4.3 soon.

@woody-apple
Copy link
Contributor

/rebase

Copy link
Contributor

@woody-apple woody-apple left a comment

Choose a reason for hiding this comment

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

Per the updated template, can you update the PR here?

#### Problem
What is being fixed?  Examples:
* Fix crash on startup
* Fixes #12345 12345 Frobnozzle is leaky (exactly like that, so GitHub will auto-close the issue).

#### Change overview
What's in this PR

#### Testing
How was this tested? (at least one bullet point required)
    • If unit tests were added, how do they cover this issue?
    • If unit tests existed, how were they fixed/modified to prevent this in future?
    • If integration tests were added, how do they verify this change?
    • If manually tested, what platforms controller and device platforms were manually tested, and how?
    • If no testing is required, why not?

@wqx6
Copy link
Contributor Author

wqx6 commented May 21, 2021

esp32c3 support needs idf version v4.3 or later, but pigweed app build fail with idf v4.3, error log shows

portGET_ARGUMENT_COUNT() result does not match for 0 arguments

will update after this PR merged

@wqx6 wqx6 force-pushed the add_ESP32C3DevKitM-support branch from dd714a7 to 4d947e8 Compare May 26, 2021 03:29
@wqx6 wqx6 force-pushed the add_ESP32C3DevKitM-support branch from 4d947e8 to f423381 Compare June 1, 2021 10:05
@wqx6 wqx6 force-pushed the add_ESP32C3DevKitM-support branch from f423381 to f6209fe Compare June 1, 2021 11:26
@wqx6 wqx6 changed the title [WIP] Add ESP32C3 DevKitM support to all-clusters-app [ESP] Add ESP32C3 DevKitM support to all-clusters-app Jun 1, 2021
@@ -10,3 +10,6 @@ pygdbmi<=0.9.0.2
reedsolo>=1.5.3,<=1.5.4
bitstring>=3.1.6
ecdsa>=0.16.0
kconfiglib==13.7.1
construct==2.10.54
python-socketio<5
Copy link
Contributor

Choose a reason for hiding this comment

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

@mspang Can you please review the scripts related changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the problem with version 5?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @mspang I was just referring to the generic changes in requirements.txt for ESP32. Nothing specific :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

If there is nothing specific then please do not use a less-than requirement.

Copy link
Contributor

Choose a reason for hiding this comment

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

As documented there: https://github.com/espressif/esp-idf/blob/v4.3/requirements.txt#L20, there was some compatible issue. We will check with our tools team, and update upstream if the issue has been fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for bug link.

@dhrishi
Copy link
Contributor

dhrishi commented Jun 18, 2021

Current PR only supports ESP32C3 for all-clusters-app now, the other apps' support will be added in a separate PR

Sure! Please raise a Github issue to track this

@wqx6 wqx6 force-pushed the add_ESP32C3DevKitM-support branch 4 times, most recently from e09c5df to 26fa994 Compare June 21, 2021 09:45
@wqx6 wqx6 force-pushed the add_ESP32C3DevKitM-support branch 3 times, most recently from 8e79787 to 24d9461 Compare June 21, 2021 10:52
@woody-apple
Copy link
Contributor

/rebase

@woody-apple
Copy link
Contributor

@wqx6 wqx6 force-pushed the add_ESP32C3DevKitM-support branch from 1198ebe to ccccca6 Compare June 22, 2021 03:47
@dhrishi
Copy link
Contributor

dhrishi commented Jun 23, 2021

@wqx6 Please rebase and resolve conflicts.

@wqx6 wqx6 force-pushed the add_ESP32C3DevKitM-support branch from ccccca6 to 5ad009a Compare June 23, 2021 07:19
@@ -10,3 +10,6 @@ pygdbmi<=0.9.0.2
reedsolo>=1.5.3,<=1.5.4
bitstring>=3.1.6
ecdsa>=0.16.0
kconfiglib==13.7.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Can it be >= ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same as the file requirements.txt in our ESP-IDF Release v4.3.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, can you fix it upstream?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @mspang, we will double check with our tools team on this specific requirement. Meantime, could we merge it as is, since it's located in requirements.esp32.txt, and we will update after our upsteam change is ready.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll hold you to that.

@@ -10,3 +10,6 @@ pygdbmi<=0.9.0.2
reedsolo>=1.5.3,<=1.5.4
bitstring>=3.1.6
ecdsa>=0.16.0
kconfiglib==13.7.1
construct==2.10.54
Copy link
Contributor

Choose a reason for hiding this comment

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

Can it be >= ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

@github-actions
Copy link

Size increase report for "esp32-example-build" from 8a53528

File Section File VM
chip-all-clusters-app.elf .dram0.data -8 -8
chip-all-clusters-app.elf .flash.rodata -8 -8
chip-all-clusters-app.elf .flash.text -396 -396
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize
.debug_loc,0,8
.debug_str,0,-4

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,3633
.debug_loc,0,1803
.debug_str,0,538
.debug_line,0,454
.debug_abbrev,0,112
.debug_ranges,0,80
[Unmapped],0,16
.dram0.data,-8,-8
.flash.rodata,-8,-8
.symtab,0,-48
.strtab,0,-76
.flash.text,-396,-396


@mspang mspang merged commit f209caf into project-chip:master Jun 24, 2021
@wqx6
Copy link
Contributor Author

wqx6 commented Jun 25, 2021

@andy31415 Can you update the chip-build-esp32 Dockerfile? We have updated the dockerfile using ESP-IDF release tag v4.3 in this PR.

@wqx6 wqx6 mentioned this pull request Jun 28, 2021
@wqx6 wqx6 deleted the add_ESP32C3DevKitM-support branch July 13, 2021 09:26
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
)

* update idf version to tag v4.3 and support esp32c3 for all-clusters-app

* fix wrong TLVWriter::Put function calling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants