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

Feature: Publish Huawei AC charger mode via MQTT #876

Merged
merged 6 commits into from
May 2, 2024

Conversation

eu-gh
Copy link

@eu-gh eu-gh commented Apr 15, 2024

Since we can set this parameter using MQTT, we should also be able to read its current value

@eu-gh
Copy link
Author

eu-gh commented Apr 15, 2024

Did the files of this project change to CRLF by default? I only added like 3 lines, yet the diff looks horrible.

@schlimmchen
Copy link
Member

No, there has been no change yet. I am looking to align line endings with the upstream project and align files only present in this repo to the other files.

What operating system are you on? Use unix2dos or dos2unix command line tools to switch line endings on Linux, the amend you change/commit and force-push.

@eu-gh
Copy link
Author

eu-gh commented Apr 15, 2024

I'm on Linux. Thanks, that did the trick!

Also I saw that you merged/reworked the sensible stuff from my old PR, big thank you for that. I was rather short on time the last couple of months.

Copy link
Member

@schlimmchen schlimmchen left a comment

Choose a reason for hiding this comment

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

Please address the second comment, where the getMode() can be implemented in the header (as its implementation is trivial) and add the const qualifier.

@@ -127,6 +127,7 @@ class HuaweiCanClass {
RectifierParameters_t * get();
uint32_t getLastUpdate();
bool getAutoPowerStatus();
uint8_t getMode();
Copy link
Member

Choose a reason for hiding this comment

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

Implement here (in header) and use const qualifier.

Copy link
Author

Choose a reason for hiding this comment

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

Implement here (in header) and use const qualifier.

Done 👍 While I'm at it, shall I also transform the getAutoPowerStatus() method in the same fashion?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, thanks. Are there more trivial getters?

Copy link
Author

Choose a reason for hiding this comment

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

getLastUpdate() and getAutoPowerStatus() -> also transformed now. I'm unsure about the generic get() for RectifierParameters_t *; as I don't really recognize that syntax and my editor complains about a type mismatch if I try to move that to the header file.

include/Huawei_can.h Show resolved Hide resolved
@schlimmchen
Copy link
Member

Also I saw that you merged/reworked the sensible stuff from my old PR, big thank you for that. I was rather short on time the last couple of months.

Thanks for your appreciation!

@eu-gh eu-gh requested a review from schlimmchen April 19, 2024 16:35
Copy link
Member

@schlimmchen schlimmchen left a comment

Choose a reason for hiding this comment

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

Let's accept the whitespace changes, as they are progress in the right direction. Other than that, the changes look good (cannot test myself, but the changes seems to be alright).

@schlimmchen schlimmchen merged commit 686b5df into hoylabs:development May 2, 2024
8 checks passed
@eu-gh eu-gh deleted the GetHuaweiModeViaMQTT branch May 20, 2024 12:48
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants