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

Fix #1147 - Exhaust temperature always zero on GB125/MC110/RC310 #1150

Merged
merged 3 commits into from Apr 2, 2023

Conversation

minusdreidb
Copy link
Contributor

@minusdreidb minusdreidb commented Mar 27, 2023

Fix always zero exhaust temperature reading by adding location telegram 0xe4, offset 31 as an additional source for the exhaust temp. and update from 0xe5 only if the value is unequal 0.

Exhaust Temperature was always zero prior to fix. On the setup

- Logano plus GB125 with Logamatic MM110 (SW-Version 02.10)
- Logamatic RC310 (SW-Version 74.03)
- MM100 (SW-Version 24.05)

the exhaust temperature ist found in telegram type 0xe4 at offset 31.
The original location in telegram 0xe5, offset 6 is always zero.
@minusdreidb minusdreidb changed the title Fix #1147 - Exhaust Temperature for Logano plus GB125 with Logamatic MC110. Fix #1147 - Exhaust temperature always zero on GB125/MC110/RC310 Mar 27, 2023
@proddy proddy requested a review from MichaelDvP March 28, 2023 06:07
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.

@proddy I'm not sure what is the right way to handle it. The exhaustTemp in E4 is now verified by @minusdreidb . The temp in E5 was just a guess and is maybe something different.
I think it's better to remove the reading in E5 and check again if users have the E5 value and can verify what it is. We can leave a comment in E5 with reference to issue #1147.
If we really want read both, @minusdreidb should add the same logic to E4-telegram to avoid toggling between 0 and value. But if both telegrams report a nonzero value we also have toggling.

@proddy
Copy link
Contributor

proddy commented Mar 28, 2023

@proddy I'm not sure what is the right way to handle it. The exhaustTemp in E4 is now verified by @minusdreidb . The temp in E5 was just a guess and is maybe something different. I think it's better to remove the reading in E5 and check again if users have the E5 value and can verify what it is. We can leave a comment in E5 with reference to issue #1147. If we really want read both, @minusdreidb should add the same logic to E4-telegram to avoid toggling between 0 and value. But if both telegrams report a nonzero value we also have toggling.

I agree. I don't really like conditional statements in the processing of the EMS telegrams. Let's remove E5 and see who starts complaining. @minusdreidb are you ok to adjust this PR?

@minusdreidb
Copy link
Contributor Author

minusdreidb commented Mar 28, 2023 via email

@minusdreidb
Copy link
Contributor Author

@proddy I made the changes and updated the PR.

@proddy
Copy link
Contributor

proddy commented Apr 2, 2023

ok with me. @MichaelDvP ?

@MichaelDvP
Copy link
Contributor

ok with me. @MichaelDvP ?

Yes.

@proddy proddy merged commit d993b35 into emsesp:dev Apr 2, 2023
@proddy proddy added this to the v3.6.0 milestone Apr 2, 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.

None yet

3 participants