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

New autocoder employing XML definition file format #527

Merged
merged 12 commits into from Apr 21, 2019
Merged

Conversation

cdjackson
Copy link
Member

@cdjackson cdjackson commented Feb 9, 2019

This PR is a rewrite of the autocoder. Primarily, it changes from an MD format to an XML format. The MD file was getting unmanageable in size as more clusters are added, and it is also more difficult to parse and add new constructs.

This may change the name of some methods due to the way the file is parsed - eg Iascie is now formatted in methods/fields as IasCie and ZoneID is now ZoneId so this may break some implementations (sorry!).

There may also be some changes to commands and attributes as these are now marked with a direction (eg wether an attribute resides in the client or server). This is one of the drivers behind this change - ie to allow a future enhancement which supports different attributes for clients and servers, as required by some clusters.

Currently this PR adds attributes for the server. This needs to be resolved as the OTA is implemented as a client on the device, so this will cause it to not work right now!

@hsudbrock FYI. I still need to test this - it generates code that is pretty much the same as before, but there might be some changes...

Signed-off-by: Chris Jackson chris@cd-jackson.com

@codecov
Copy link

codecov bot commented Feb 9, 2019

Codecov Report

❗ No coverage uploaded for pull request base (dev-1.2.0@f14fc50). Click here to learn what that means.
The diff coverage is 47.56%.

Impacted file tree graph

@@             Coverage Diff              @@
##             dev-1.2.0     #527   +/-   ##
============================================
  Coverage             ?   24.49%           
  Complexity           ?     3003           
============================================
  Files                ?     1298           
  Lines                ?    45306           
  Branches             ?     1917           
============================================
  Hits                 ?    11097           
  Misses               ?    33629           
  Partials             ?      580
Impacted Files Coverage Δ Complexity Δ
...gbee/zcl/clusters/iasace/GetZoneIdMapResponse.java 0% <ø> (ø) 0 <0> (?)
.../keyestablishment/EphemeralDataRequestCommand.java 0% <ø> (ø) 0 <0> (?)
...igbee/zcl/clusters/doorlock/UnlockDoorCommand.java 0% <ø> (ø) 0 <0> (?)
...e/zcl/clusters/general/WriteAttributesCommand.java 0% <ø> (ø) 0 <0> (?)
...ems/zigbee/zcl/clusters/metering/ChangeSupply.java 0% <ø> (ø) 0 <0> (?)
...onseandloadcontrol/CancelAllLoadControlEvents.java 0% <ø> (ø) 0 <0> (?)
...keyestablishment/ConfirmKeyDataRequestCommand.java 0% <ø> (ø) 0 <0> (?)
...bee/zcl/clusters/ZclPowerConfigurationCluster.java 0% <ø> (ø) 0 <0> (?)
.../clusters/basic/ResetToFactoryDefaultsCommand.java 0% <ø> (ø) 0 <0> (?)
.../demandresponseandloadcontrol/EventStatusEnum.java 0% <ø> (ø) 0 <0> (?)
... and 196 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f14fc50...d364c8e. Read the comment docs.

@cdjackson
Copy link
Member Author

This pull request introduces 2 alerts and fixes 1 when merging caa6133 into da475fa - view on LGTM.com

new alerts:

  • 1 for Useless comparison test
  • 1 for Dereferenced variable may be null

fixed alerts:

  • 1 for Useless comparison test

Comment posted by LGTM.com

@cdjackson
Copy link
Member Author

This pull request introduces 2 alerts and fixes 10 when merging 06ad4e2 into 2cb1315 - view on LGTM.com

new alerts:

  • 1 for Useless comparison test
  • 1 for Dereferenced variable may be null

fixed alerts:

  • 7 for Useless comparison test
  • 1 for Unused format argument
  • 1 for Missing format argument
  • 1 for Useless null check

Comment posted by LGTM.com

@cdjackson cdjackson force-pushed the XML-autocoder branch 3 times, most recently from d5848e9 to ccd804a Compare February 9, 2019 19:42
@zsmartsystems zsmartsystems deleted a comment Feb 9, 2019
@cdjackson
Copy link
Member Author

This pull request introduces 2 alerts and fixes 10 when merging ccd804a into 2cb1315 - view on LGTM.com

new alerts:

  • 1 for Useless comparison test
  • 1 for Dereferenced variable may be null

fixed alerts:

  • 7 for Useless comparison test
  • 1 for Unused format argument
  • 1 for Missing format argument
  • 1 for Useless null check

Comment posted by LGTM.com

@t-8ch
Copy link
Contributor

t-8ch commented Feb 9, 2019

Is it intentional, that the generated code is commited to the repo?
I would have expected the code to be generated at build-time during the generate-sources phase.

@t-8ch
Copy link
Contributor

t-8ch commented Feb 9, 2019

FYI, there is a nice library to support with java code generation: https://github.com/square/javapoet
The current generation seems fine, just to make you aware of it.

@@ -0,0 +1,108 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

This class seems not to be used. Is something missing from the PR?

@cdjackson
Copy link
Member Author

Is it intentional, that the generated code is commited to the repo?

I have been thinking about changing to this concept, but it's always been this way, and I've not bothered to change it. If you want to provide a PR, and it works ok, then that would be great :)

FYI, there is a nice library to support with java code generation: https://github.com/square/javapoet

Thanks. The autocoders in this PR aren't new so I don't really anticipate changing this.

This class seems not to be used. Is something missing from the PR?

Thanks - looks like I moved the functions out of the class at some point in the past, so it's not used. I've removed it.

@cdjackson
Copy link
Member Author

This pull request introduces 2 alerts and fixes 10 when merging e6df9bd into 2cb1315 - view on LGTM.com

new alerts:

  • 1 for Useless comparison test
  • 1 for Dereferenced variable may be null

fixed alerts:

  • 7 for Useless comparison test
  • 1 for Unused format argument
  • 1 for Missing format argument
  • 1 for Useless null check

Comment posted by LGTM.com

@cdjackson
Copy link
Member Author

This pull request introduces 2 alerts and fixes 10 when merging 4be095b into 263da31 - view on LGTM.com

new alerts:

  • 1 for Useless comparison test
  • 1 for Dereferenced variable may be null

fixed alerts:

  • 7 for Useless comparison test
  • 1 for Unused format argument
  • 1 for Missing format argument
  • 1 for Useless null check

Comment posted by LGTM.com

@zsmartsystems zsmartsystems deleted a comment Feb 16, 2019
@hsudbrock
Copy link
Contributor

@cdjackson I have started testing with the code from this PR; so far, things look good.

We discussed that it could be good to include these changes together with the changes necessary for the manufacturer-specific clusters, so I will simply rebase my PR for the manufacturer-specific clusters on this PR.

@cdjackson
Copy link
Member Author

cdjackson commented Feb 22, 2019 via email

@hsudbrock
Copy link
Contributor

Chris, I am currently developing in openHAB based on this branch, also for testing it some more. What would be helpful is support for client-side attributes, as otherwise discoveries always run into a NPE (the node property discoverer tries to read the FW version, which fails as this is a client-side attribute that is not yet written into the attribute list of the ZclOtaUpgradeCluster).

So, if you happen to push updates to this branch, this would be helpful for me :) (But I can work around this in the time being...)

out.println(" public boolean isTransactionMatch(ZigBeeCommand request, ZigBeeCommand response) {");
if (command.response.matchers.isEmpty()) {
out.println(" return (response instanceof " + command.response.command + ")");
out.println(" & ((ZdoRequest) request).getDestinationAddress().equals((("
Copy link
Contributor

Choose a reason for hiding this comment

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

Small typo: This should be && here (one & is missing).

@cdjackson
Copy link
Member Author

This pull request introduces 2 alerts and fixes 8 when merging 5ada35f into ec8c99e - view on LGTM.com

new alerts:

  • 1 for Useless comparison test
  • 1 for Dereferenced variable may be null

fixed alerts:

  • 7 for Useless comparison test
  • 1 for Useless null check

Comment posted by LGTM.com

@cdjackson
Copy link
Member Author

This pull request introduces 3 alerts and fixes 8 when merging ea3327e into cfca6e7 - view on LGTM.com

new alerts:

  • 1 for Useless comparison test
  • 1 for Dereferenced variable may be null
  • 1 for Unused format argument

fixed alerts:

  • 7 for Useless comparison test
  • 1 for Useless null check

Comment posted by LGTM.com

@cdjackson cdjackson force-pushed the XML-autocoder branch 2 times, most recently from bb47da3 to 22f8c83 Compare April 20, 2019 07:58
@cdjackson
Copy link
Member Author

This pull request fixes 8 alerts when merging 22f8c83 into cfca6e7 - view on LGTM.com

fixed alerts:

  • 7 for Useless comparison test
  • 1 for Useless null check

Comment posted by LGTM.com

@cdjackson
Copy link
Member Author

This pull request fixes 8 alerts when merging d47fce4 into 1671d47 - view on LGTM.com

fixed alerts:

  • 7 for Useless comparison test
  • 1 for Useless null check

Comment posted by LGTM.com

@cdjackson
Copy link
Member Author

This pull request fixes 8 alerts when merging 5c4c587 into edcce46 - view on LGTM.com

fixed alerts:

  • 7 for Useless comparison test
  • 1 for Useless null check

Comment posted by LGTM.com

Signed-off-by: Chris Jackson <chris@cd-jackson.com>
Signed-off-by: Chris Jackson <chris@cd-jackson.com>
Signed-off-by: Chris Jackson <chris@cd-jackson.com>
Signed-off-by: Chris Jackson <chris@cd-jackson.com>
Signed-off-by: Chris Jackson <chris@cd-jackson.com>
Signed-off-by: Chris Jackson <chris@cd-jackson.com>
Signed-off-by: Chris Jackson <chris@cd-jackson.com>
Signed-off-by: Chris Jackson <chris@cd-jackson.com>
Signed-off-by: Chris Jackson <chris@cd-jackson.com>
…methods

Signed-off-by: Chris Jackson <chris@cd-jackson.com>
Signed-off-by: Chris Jackson <chris@cd-jackson.com>
Signed-off-by: Chris Jackson <chris@cd-jackson.com>
@cdjackson
Copy link
Member Author

This pull request fixes 8 alerts when merging d364c8e into f14fc50 - view on LGTM.com

fixed alerts:

  • 7 for Useless comparison test
  • 1 for Useless null check

Comment posted by LGTM.com

@cdjackson cdjackson merged commit 4904774 into dev-1.2.0 Apr 21, 2019
cdjackson added a commit that referenced this pull request Apr 23, 2019
New autocoder employing XML definition file format

Signed-off-by: Chris Jackson <chris@cd-jackson.com>
cdjackson added a commit that referenced this pull request Apr 27, 2019
New autocoder employing XML definition file format

Signed-off-by: Chris Jackson <chris@cd-jackson.com>
cdjackson added a commit that referenced this pull request Apr 28, 2019
New autocoder employing XML definition file format

Signed-off-by: Chris Jackson <chris@cd-jackson.com>
cdjackson added a commit that referenced this pull request May 4, 2019
New autocoder employing XML definition file format

Signed-off-by: Chris Jackson <chris@cd-jackson.com>
cdjackson added a commit that referenced this pull request May 7, 2019
New autocoder employing XML definition file format

Signed-off-by: Chris Jackson <chris@cd-jackson.com>
cdjackson added a commit that referenced this pull request May 10, 2019
New autocoder employing XML definition file format

Signed-off-by: Chris Jackson <chris@cd-jackson.com>
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

3 participants