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

ESP32: Add fixes for lock-app #6658

Merged
merged 1 commit into from
May 27, 2021

Conversation

sweetymhaiske
Copy link
Contributor

@sweetymhaiske sweetymhaiske commented May 11, 2021

Problem

  • Add missing fixes for lock-app ESP32.

Change overview

  1. Added missing cluster ID and endpoint ID check for on-off cluster.
  2. Make lock-app as minimal as possible hence removed M5Stack support.
  3. Map on/off cluster to lock/unlock logic

Testing

  • Using chip-tool first commissioned the device via SoftAP
  • When On/Off command was sent using On/Off cluster, then LOCK LED switch on/off depending upon value sent to the cluster
  • When Button1 is pressed the LOCK LED changes its state
  • When Button2 has pressed then factory reset happens

@sweetymhaiske sweetymhaiske changed the title ESP32: Add fixes for lock-app [WIP] ESP32: Add fixes for lock-app May 11, 2021
@woody-apple
Copy link
Contributor

/rebase

@woody-apple woody-apple changed the title [WIP] ESP32: Add fixes for lock-app ESP32: Add fixes for lock-app May 14, 2021
@woody-apple
Copy link
Contributor

Removing WIP title so Pull Approve is happy, this appears ready?

@woody-apple
Copy link
Contributor

@msandstedt @andy31415 ?

@dhrishi
Copy link
Contributor

dhrishi commented May 15, 2021

Removing WIP title so Pull Approve is happy, this appears ready?

@woody-apple I think a few changes are pending. Should we move it back to WIP, so that it isn't merged?

@dhrishi dhrishi changed the title ESP32: Add fixes for lock-app [WIP] ESP32: Add fixes for lock-app May 15, 2021
@woody-apple
Copy link
Contributor

Marking this as a draft given it's WIP then :) Please un-draft it when you're ready!

@woody-apple woody-apple marked this pull request as draft May 17, 2021 18:26
@sweetymhaiske sweetymhaiske force-pushed the fix_lock_app branch 2 times, most recently from 640e319 to e5cbb54 Compare May 18, 2021 13:16
@sweetymhaiske sweetymhaiske marked this pull request as ready for review May 18, 2021 14:13
@sweetymhaiske sweetymhaiske changed the title [WIP] ESP32: Add fixes for lock-app ESP32: Add fixes for lock-app May 18, 2021
@andy31415
Copy link
Contributor

andy31415 commented May 19, 2021

Please add a section in the summary for how this was tested - what steps/tools did you use to validate the functionality update?

@andy31415
Copy link
Contributor

@sweetymhaiske - this is almost ready to go in. Please update the PR summary with a 'how did you test this' section before we can merge.

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?

@sweetymhaiske
Copy link
Contributor Author

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?

Done

@sweetymhaiske
Copy link
Contributor Author

@sweetymhaiske - this is almost ready to go in. Please update the PR summary with a 'how did you test this' section before we can merge.

Done

@sweetymhaiske sweetymhaiske requested a review from woody-apple May 22, 2021 07:15
@dhrishi
Copy link
Contributor

dhrishi commented May 24, 2021

@sweetymhaiske Is there anything pending here?

@sweetymhaiske
Copy link
Contributor Author

@sweetymhaiske Is there anything pending here?

No.

@dhrishi
Copy link
Contributor

dhrishi commented May 25, 2021

@woody-apple Can you please take a look?

@andy31415 andy31415 dismissed woody-apple’s stale review May 27, 2021 13:51

Dismissing review for "summary content was updated according to the template"

@andy31415 andy31415 merged commit 4262811 into project-chip:master May 27, 2021
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
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.

8 participants