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.

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

Generate ZCL code for Android #7439

Merged
merged 4 commits into from
Jun 21, 2021
Merged

Generate ZCL code for Android #7439

merged 4 commits into from
Jun 21, 2021

Conversation

austinh0
Copy link
Contributor

@austinh0 austinh0 commented Jun 7, 2021 •
edited
Loading

Problem

Android has no generated clusters support.

Change overview

  • Refactor stack locking into StackUnlock.h so it can be reused
  • Create ZAP templates and helper.js that generate src/controller/java/gen/ChipClusters.java and src/controller/java/gen/ChipClusters-JNI.cpp
    • Attribute methods and callbacks still TODO
  • Add Java paths to zap_regen_all script
  • Use NetworkCommissioningCluster and OnOffCluster in Android CHIPTool

Testing

  • Successfully went through network commissioning and was able to perform on/off control for ESP32.

@restyled-io restyled-io bot mentioned this pull request Jun 7, 2021
Closed
@austinh0 austinh0 requested review from Damian-Nordic and removed request for vidhis88 June 7, 2021 23:22
@pullapprove pullapprove bot requested a review from vidhis88 June 7, 2021 23:22
@austinh0
Copy link
Contributor Author

austinh0 commented Jun 8, 2021

Not sure why the ZAP templates action is failing - I checked out the "Generated files" commit, re-ran zap_regen_all.py, and found no differences for ChipClusters.java. Also, it passes for "Use clusters in Android CHIPTool": https://github.com/austinh0/connectedhomeip/runs/2768887797

@Damian-Nordic
Copy link
Contributor

I tested the change and it works correctly with Thread devices, too, but please fix the lock issue.

@bzbarsky-apple
Copy link
Contributor

Not sure why the ZAP templates action is failing

@austinh0 it's failing because ChipClusters-java.zapt has:

   public static abstract class BaseChipCluster {

while ChipClusters.java in the PR has has:

   public abstract static class BaseChipCluster {

(note different order of qualifiers). There are other differences too. When I run zap_regen_all.py locally I get a change to the public static abstract class BaseChipCluster version and various whitespace changes.

Which makes me suspect that the restyler you are running and the one CI is running (which might match the one I am running) might be different? Best guess so far on that, at least...

@restyled-io restyled-io bot mentioned this pull request Jun 9, 2021
Closed
@austinh0 austinh0 requested a review from Damian-Nordic June 9, 2021 04:45
@austinh0
Copy link
Contributor Author

austinh0 commented Jun 9, 2021

I have resolved the comments, but it seems I need to make generate.py format Java files for the CI to pass. I will do this in a separate PR, then merge this one.

@woody-apple
Copy link
Contributor

/rebase

1 similar comment
@woody-apple
Copy link
Contributor

/rebase

@austinh0 austinh0 mentioned this pull request Jun 17, 2021
Merged
@austinh0
Copy link
Contributor Author

ZAP check is passing now, this is ready to review again.

@Damian-Nordic @saurabhst @mspang

andy31415
andy31415 reviewed Jun 21, 2021
View reviewed changes
src/controller/java/JniReferences.h

#include <core/CHIPError.h>
class JniReferences
Copy link
Contributor

Choose a reason for hiding this comment

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

Everything in this class seems to be static. Would a namespace be more appropriate here?

Alternatively, this should be a singleton class with some ::Instance method so that not all methods are static.

Copy link
Contributor Author

@austinh0 austinh0 Jun 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

#7802

@mspang mspang merged commit 6e75320 into project-chip:master Jun 21, 2021
This was referenced Jun 21, 2021
Merged
Merged
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
@austinh0
Generate ZCL code for Android (project-chip#7439)
d999c01 
* Create StackLock.h, containing StackLockGuard (formerly ScopedPthread) and StackUnlockGuard

* Add ZAP templates for accessing clusters through JNI

* Generated files and java/BUILD changes to include them

* Update Android CHIPTool to use cluster commands.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andy31415 andy31415 andy31415 left review comments

@Damian-Nordic Damian-Nordic Damian-Nordic approved these changes

@woody-apple woody-apple woody-apple approved these changes

@mspang mspang mspang approved these changes

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

@jelderton jelderton Awaiting requested review from jelderton

@jmartinez-silabs jmartinez-silabs Awaiting requested review from jmartinez-silabs

@LuDuda LuDuda Awaiting requested review from LuDuda

@saurabhst saurabhst Awaiting requested review from saurabhst

@vivien-apple vivien-apple Awaiting requested review from vivien-apple

@vidhis88 vidhis88 Awaiting requested review from vidhis88

Assignees
No one assigned
Labels
android controller review - approved scripts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@austinh0 @Damian-Nordic @bzbarsky-apple @woody-apple @mspang @andy31415