-
Notifications
You must be signed in to change notification settings - Fork 9
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
Alternator: Engine Temperature: Added support #1187
Conversation
text: CommonWords.temperature | ||
dataItem.uid: root.bindPrefix + "/Engine/Temperature" | ||
allowed: defaultAllowed && dataItem.isValid | ||
onHeightChanged: console.trace() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftover debugging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably. new to the GUI version 2, so copied some earlier segments.
@@ -85,6 +85,13 @@ ObjectModel { | |||
allowed: defaultAllowed && dataItem.isValid | |||
} | |||
|
|||
ListTemperatureItem { | |||
text: CommonWords.temperature |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good, but I don't see '.../Engine/Temperature' on the bus (I do see '.../Engine/Speed'). Does this need other customized firmware to work? I am running the latest 'Testing' release, v3.33~1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is a new dbus path that has been added through the ve.can driver here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be merged? Has the ve.can driver change been included in the existing releases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is in the main branch of vecan-dbus, however that has not yet been merged into VenusOS beta. It is on the todo list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nmbath I was going to merge this, but noticed that we now have two "Temperature" entries on this page. On other pages where we have multiple temperature settings, they have names like "Battery temperature" and "Oil temperature" to distinguish between them. Could you do the same here, and have different text for each temperature entry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blammit I have differentiated using sTrId("dc_alternator_temperature")
and sTrId("alternator_engine_temperature")
however not sure what else is needed to define these strings or where they are defined. Please advise if anything else is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nmbath it just needs the Engineering English strings, like this:
//% "Alternator temperature"
text: qsTrId("dc_alternator_temperature")
and
//% "Engine temperature"
text: qsTrId("alternator_engine_temperature")
(If those are the strings that should be displayed.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blammit Many thanks. Done and pushed an udpate.
There was a problem hiding this 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 (once the leftover debugging is removed)
I have updated to remove the console.trace(). |
I love the idea of displaying the temp on the alternator tile. However one suggestion is that alternator temp would likely be more useful than the engine temp for this. Devices like Wakespeed and Zeus include a temperature probe and already make the alternator temp available in the Cerbo software. |
Alternator temp is already displayed, as long as it has been supplied by the controller. In all cases this is displayed on the details page. |
Understood, makes sense. I was thinking this was adding the temp display on the Overview tile, similar to the battery temp display. Now I understand you are adding it in the Details page for the alternator. Great addition! |
@nmbath please ping me or Daniel when the required version of vecan-dbus has made its way into venus, so we can merge this one. Thanks! |
@chriadam this is now in V3.50beta. It is in vecan-dbus v3.29 and that was added to v3.50~1 according to the todo list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs an update to the temperature names, as commented.
Added support to display engine temperature.