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

added dhw alternating operation #901

Merged
merged 6 commits into from
Jan 22, 2023
Merged

added dhw alternating operation #901

merged 6 commits into from
Jan 22, 2023

Conversation

pswid
Copy link
Contributor

@pswid pswid commented Jan 10, 2023

In the thermostat menu options look like this:

dhw alternating
prioritise dhw
prioritise heating

pswid added 2 commits January 10, 2023 11:50
Because of compiler error, in Polish translation,  we don't use uppercase boolean option "WŁĄCZONO/WYŁĄCZONO" but "wł./wył." instead.
src/helpers.cpp Outdated Show resolved Hide resolved
@proddy proddy requested review from proddy and MichaelDvP January 12, 2023 17:12
Copy link
Contributor

@MichaelDvP MichaelDvP left a comment

Choose a reason for hiding this comment

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

I see the intension to add "OFF" to the bool input with pl language, but we have to keep the toLower(value) to match all mixed case writings like "True", "On".
Please re-add bool_str = toLower(value) and change in the if's to bool_str == toLower(Helpers::translated_word(FL_(OFF)) then all options should work.

pswid added 2 commits January 13, 2023 21:45
now in mqtt discovery topics you can see e.g:
"uniq_id": "thermostat_OG1_mode_type" (for Polish lang.)

but should be:
"uniq_id": "thermostat_hc1_mode_type",
Because of compiler error, in Polish translation,  we don't use uppercase boolean option "WŁĄCZONO/WYŁĄCZONO" but "wł./wył." instead.
@pswid
Copy link
Contributor Author

pswid commented Jan 15, 2023

Done. Sorry for delay. It works but I noticed that toLower() and toUpper() do not work on Polish characters (ĄĘĆŁŃÓŚŹŻ). It should be fixed but I don't know how. I read somewhere that locale needs to be set properly.

@MichaelDvP
Copy link
Contributor

I think you have to check the special charcater here explicide:

std::transform(lc.begin(), lc.end(), lc.begin(), [](unsigned char c) { return std::tolower(c); });

@pswid
Copy link
Contributor Author

pswid commented Jan 15, 2023

toupper() is used e.g. here:

F_name[0] = toupper(F_name[0]); // capitalize first letter

and it doesn't work. The first letter of HA friendy names starting with Polish character is not capitalized (e.g. "łączny czas grzania"), so I assume that std::tolower() will not work also.

@MichaelDvP
Copy link
Contributor

Yes the std::tolower only converts A-Z. I thought of something like a Helpers::tolower:
(i have not tested if it works)

int Helpers::tolower(int _c) {
    switch (_c) {
    case 'Ż': return 'ż';
    // insert other special characters here
    default: return std::tolower(_c);
    }
}

std::string Helpers::toLower(std::string const & s) {
    std::string lc = s;
    std::transform(lc.begin(), lc.end(), lc.begin(), [](unsigned char c) { return tolower(c); }); // use own tolower() here
    return lc;
}

@proddy
Copy link
Contributor

proddy commented Jan 15, 2023

Yes the std::tolower only converts A-Z. I thought of something like a Helpers::tolower: (i have not tested if it works)

int Helpers::tolower(int _c) {
    switch (_c) {
    case 'Ż': return 'ż';
    // insert other special characters here
    default: return std::tolower(_c);
    }
}

std::string Helpers::toLower(std::string const & s) {
    std::string lc = s;
    std::transform(lc.begin(), lc.end(), lc.begin(), [](unsigned char c) { return tolower(c); }); // use own tolower() here
    return lc;
}

that would work. Or just don't bother with toLower/Upper because of the different charsets. I don't think we'll miss much.

@pswid
Copy link
Contributor Author

pswid commented Jan 16, 2023

I wouldn't want special characters to be ignored. This can cause various problems in other places, especially if more languages using special characters will be added in the future.

The main cause of problems with special characters (e.g. Polish letters) is due to the fact that in UTF-8 they are not encoded in 1 byte, but in 2 bytes (in UTF-8 there are characters that require even 4 bytes!).

My commit 4f05dda doesn't "break" anything, but fixes a bug with the boolean "wł./wył." option, so I think it can be merged (and possibly improved in the future).

Since there are currently no entities starting in Polish with a special character other than "ł", temporary solution could by replacing

F_name[0] = toupper(F_name[0]); // capitalize first letter
by:

    if (F_name[0] == 0xC5 && F_name[1] == 0x82)  //if first letter is Polish "ł" (UTF-8: 0xC5 0x82)...
        F_name[1] = 0x81;   //change it to "Ł" (UTF-8: 0xC5 0x81)
    else
        F_name[0]     = toupper(F_name[0]); // capitalize first letter

Ultimately, however, own toupper & tolower procedures should be created that will also work with special characters (are not based on 8-bit variables such as char).

src/helpers.cpp Outdated
switch (*c) {
case 0xC3:
if (*(c + 1) == 0xB3) //ó (0xC3,0xB3) -> Ó (0xC3,0x93)
*(c + 1) = 0x93;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can change this to match more special characters to:

        // grave, acute, circumflex, diaeresis, etc.
        if ((*(c + 1) >= 0xA0) && (*(c + 1) <= 0xBE)) {
            *(c + 1) -= 0x20;
        }

Copy link
Contributor

Choose a reason for hiding this comment

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

when this is done I'll merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. sorry for the delay

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

ok to merge @MichaelDvP ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

@proddy proddy merged commit 485195e into emsesp:dev Jan 22, 2023
@proddy proddy added this to the v3.5.0 milestone Jan 22, 2023
proddy added a commit that referenced this pull request Jan 22, 2023
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.

3 participants