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

Refactored Shelly 2.5 to use HttpBridge #2573

Merged
merged 15 commits into from
Jun 1, 2024

Conversation

Sn0w3y
Copy link
Contributor

@Sn0w3y Sn0w3y commented Mar 11, 2024

No description provided.

Copy link

Code Coverage

Copy link

Code Coverage

@Sn0w3y Sn0w3y changed the title Refactored to use HttpBridge Refactored Shelly 2.5 to use HttpBridge Mar 18, 2024
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.

Many topics appear over and over again in reviews. I believe we should eventually create an AbstractShelly class that combines all of the generic logic...

Copy link

github-actions bot commented Apr 1, 2024

Code Coverage

@Sn0w3y
Copy link
Contributor Author

Sn0w3y commented Apr 1, 2024

@sfeilmeier do you think it makes sense to sum up the meter Values and set _setActivePower(summedUpValues) for this Shelly ?

@Sn0w3y Sn0w3y requested a review from sfeilmeier April 1, 2024 14:13
Copy link

github-actions bot commented Apr 1, 2024

Code Coverage

1 similar comment
Copy link

github-actions bot commented Apr 1, 2024

Code Coverage

Copy link

Code Coverage

@Sn0w3y
Copy link
Contributor Author

Sn0w3y commented Apr 16, 2024

@sfeilmeier shoul be everything set now

If we Merge all of the Shelly Impls i could start working on a Generic Shelly Class like Abstract it as we bespoke somehwhere

Copy link

Code Coverage

Copy link

Code Coverage

Copy link

Code Coverage

Copy link

Code Coverage

Copy link

Code Coverage

this._setRelay1(relay1State.relayIsOn());
this._setRelay2(relay2State.relayIsOn());

this._setRelay1Overtemp(relay1State.overtemp());
Copy link
Contributor

Choose a reason for hiding this comment

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

This potentially produces a NullPointerException

} else if (i == 1) {
relay2State = parseRelay(JsonUtils.getAsJsonObject(relays.get(1)));
}
}
this._setSlaveCommunicationFailed(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is redundant/illogical

JsonElement jsonElement = JsonUtils.getAsJsonElement(result);
final var relays = JsonUtils.getAsJsonArray(jsonElement, "relays");

for (int i = 0; i < 2; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This loop is not required

*
* @return the Channel
*/
public default BooleanWriteChannel getRelay2OverpowerChannel() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a WriteChannel.

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.

Leider waren immer noch sehr viele Programmierfehler im Code. Ich muss die Qualitätsanforderungen hoch halten, deshalb kann ich das dann so nicht einfach mergen. So kommen wir mit den Shellys leider nur langsam voran. Von umfangreichen JUnit-Tests (z. B. parsing tests für die JSON-Response) spreche ich noch gar nicht.

Diesen PR habe ich jetzt nochmal selbst einigermaßen bereinigt. Bitte schau dir mal meine Commits und Kommentare an.

Copy link

github-actions bot commented Jun 1, 2024

Code Coverage

@sfeilmeier sfeilmeier merged commit 6937f29 into OpenEMS:develop Jun 1, 2024
2 checks passed
@Sn0w3y Sn0w3y deleted the refactor/Shelly25 branch June 3, 2024 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants