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

Implement Shelly 3EM #2519

Merged
merged 17 commits into from
Feb 24, 2024
Merged

Implement Shelly 3EM #2519

merged 17 commits into from
Feb 24, 2024

Conversation

Sn0w3y
Copy link
Contributor

@Sn0w3y Sn0w3y commented Feb 5, 2024 •
edited
Loading

Link to Shelly Product

This is a 3-phase Meter, which can be used as a Grid Meter for example.

@sfeilmeier
Copy link
Contributor

@michaelgrill Please check usage of HTTP-Bridge

@Sn0w3y
Copy link
Contributor Author

Sn0w3y commented Feb 6, 2024

@sfeilmeier the other Error on Test was because of AbbreviationAsWordInName - the name is 3EM - i would be glad if we could keep this as it is as this is the original name - what do you think?

@Sn0w3y
Change of Energy Calculation to handle every Case
5bed9b6 
Calculate the Energy values from ActivePower.
@Sn0w3y
Update IoShellyPlus3EMImpl.java
c333abe 
Added this. for Checkstyle this.setValuesForPhase(i + 1, voltage, current, power);
michaelgrill
michaelgrill requested changes Feb 7, 2024
View reviewed changes
Copy link
Contributor

@michaelgrill michaelgrill left a comment

Choose a reason for hiding this comment

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

General usage of HttpBridge is fine. Had some other things i noticed and also format your code 🙂 (CTRL + SHIFT + F in Eclipse)

io.openems.edge.io.shelly/src/io/openems/edge/io/shelly/shellyplus3em/IoShellyPlus3EMImpl.java Outdated Show resolved Hide resolved
io.openems.edge.io.shelly/src/io/openems/edge/io/shelly/shellyplus3em/IoShellyPlus3EMImpl.java Outdated Show resolved Hide resolved
io.openems.edge.io.shelly/src/io/openems/edge/io/shelly/shellyplus3em/IoShellyPlus3EMImpl.java Outdated Show resolved Hide resolved
io.openems.edge.io.shelly/src/io/openems/edge/io/shelly/shellyplus3em/IoShellyPlus3EMImpl.java Outdated Show resolved Hide resolved
@sfeilmeier
Copy link
Contributor

@sfeilmeier the other Error on Test was because of AbbreviationAsWordInName - the name is 3EM - i would be glad if we could keep this as it is as this is the original name - what do you think?

It's very often like this unfortunately. It's possible to ignore the Checkstyle error (using // CHECKSTYLE:OFF - and :ON, but generally you should rename it to e.g. 3em

@Sn0w3y
Copy link
Contributor Author

Sn0w3y commented Feb 7, 2024

@sfeilmeier @michaelgrill should all be done and ready

@Sn0w3y
Copy link
Contributor Author

Sn0w3y commented Feb 7, 2024

Done ! @michaelgrill :)

michaelgrill
michaelgrill requested changes Feb 13, 2024
View reviewed changes
Copy link
Contributor

@michaelgrill michaelgrill left a comment

Choose a reason for hiding this comment

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

usage in general looks fine only some side notes and did formatting change again? 🤔
after the merge of #2539 we should be able to merge this one as well

io.openems.edge.io.shelly/src/io/openems/edge/io/shelly/shellyplus3em/IoShellyPlus3emImpl.java Outdated Show resolved Hide resolved
io.openems.edge.io.shelly/src/io/openems/edge/io/shelly/shellyplus3em/IoShellyPlus3emImpl.java Outdated Show resolved Hide resolved
@Sn0w3y
Copy link
Contributor Author

Sn0w3y commented Feb 13, 2024

@michaelgrill should be done now ! :) ready to check and review again :)

If your new HttpBridge is merged it would be great if this also would be merged ? :)

@Sn0w3y
Copy link
Contributor Author

Sn0w3y commented Feb 21, 2024

as the HttpBridge works now maybe up ?

michaelgrill
michaelgrill approved these changes Feb 22, 2024
View reviewed changes
Copy link
Contributor

@michaelgrill michaelgrill left a comment

Choose a reason for hiding this comment

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

Looks good to me. 👍 ty
(i was waiting for #2539 (review) to be merged)

sfeilmeier
sfeilmeier reviewed Feb 22, 2024
View reviewed changes
Copy link
Contributor

@sfeilmeier sfeilmeier left a comment

Choose a reason for hiding this comment

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

Please find my comments. I applied all suggested changes. Can you please validate that everything still works?

io.openems.edge.io.shelly/bnd.bnd Outdated Show resolved Hide resolved
io.openems.edge.io.shelly/src/io/openems/edge/io/shelly/shellyplus3em/Config.java Outdated Show resolved Hide resolved
io.openems.edge.io.shelly/src/io/openems/edge/io/shelly/shellyplus3em/IoShellyPlus3em.java Outdated Show resolved Hide resolved
io.openems.edge.io.shelly/src/io/openems/edge/io/shelly/shellyplus3em/IoShellyPlus3emImpl.java Outdated Show resolved Hide resolved
io.openems.edge.io.shelly/src/io/openems/edge/io/shelly/shellyplus3em/IoShellyPlus3emImpl.java Outdated Show resolved Hide resolved
io.openems.edge.io.shelly/src/io/openems/edge/io/shelly/shellyplus3em/IoShellyPlus3emImpl.java Outdated Show resolved Hide resolved
io.openems.edge.io.shelly/src/io/openems/edge/io/shelly/shellyplus3em/IoShellyPlus3emImpl.java Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

Code Coverage

@Sn0w3y
Copy link
Contributor Author

Sn0w3y commented Feb 22, 2024

I will test this and report if it works :) @sfeilmeier

@sfeilmeier
Copy link
Contributor

I will test this and report if it works :) @sfeilmeier

Thanks. Waiting for your final "Go!" to merge this.

@Sn0w3y
Update IoShelly3EmImpl.java some fixes
3c78fe4 
Needed to fix the *1000 for current and voltage.

I also added some functionalities to verify the conditions of the E-Meters in the IoShelly3Em.java :)
@Sn0w3y
Update IoShelly3Em.java Channels to match the Impl
2ef15f5
@Sn0w3y
Copy link
Contributor Author

Sn0w3y commented Feb 24, 2024

@sfeilmeier should be completed now ! I just added some functionalities - maybe just have a little look after it:

this.channel(IoShelly3Em.ChannelId.EMETER1_EXCEPTION).setNextValue(!isValid);
this.channel(IoShelly3Em.ChannelId.EMETER2_EXCEPTION).setNextValue(!isValid);
this.channel(IoShelly3Em.ChannelId.EMETER3_EXCEPTION).setNextValue(!isValid);

also added:

this.channel(IoShelly3Em.ChannelId.RELAY_OVERPOWER_EXCEPTION).setNextValue(overpower);

to see if the Relais is overpowered :)

@github-actions GitHub Actions
Copy link

Code Coverage

@Sn0w3y
Copy link
Contributor Author

Sn0w3y commented Feb 24, 2024

fixed and working :D @sfeilmeier

@github-actions GitHub Actions
Copy link

Code Coverage

@github-actions GitHub Actions
Copy link

Code Coverage

@sfeilmeier sfeilmeier merged commit 01dc664 into OpenEMS:develop Feb 24, 2024
2 checks passed
@Sn0w3y Sn0w3y deleted the ShellyPro3EM-Implementation branch February 26, 2024 21:54
@sfeilmeier sfeilmeier mentioned this pull request Apr 28, 2024
Merged
fanass-dev pushed a commit to fanass-dev/openems that referenced this pull request May 6, 2024
@Sn0w3y @fanass-dev
Implement Shelly 3EM (OpenEMS#2519)
38083df 
https://www.shelly.com/de/products/shop/shelly-3em-1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfeilmeier sfeilmeier sfeilmeier left review comments

@michaelgrill michaelgrill michaelgrill approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Sn0w3y @sfeilmeier @michaelgrill