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

Improve GPS support #1841

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

dirkjanfaber
Copy link
Contributor

  • Show the cardinal direction for the course.
  • Add a proper unit for the altitude (meter / feet)

Note that there is also a patch on veutil for altitude support.

image

- Show the cardinal direction for the course.
- Add a proper unit for the altitude (meter / feet), also to veutil.
@DanielMcInnes
Copy link
Contributor

Hi @dirkjanfaber , is there an installation where I can test this?

@dirkjanfaber
Copy link
Contributor Author

dirkjanfaber commented Jan 15, 2025

Send info on the installation via Slack.

Another thing to note is that the Venus settings currently doesn't have a general setting for Altitude (via venus-platform). To me it would seem logical to add it as /System/Units/Altitude, comparable with the Kelvin / Celsius TemperatureUnit here: https://github.com/victronenergy/venus-platform/blob/master/src/application.cpp#L237

Alternative is to add it as /Gps/AltitudeUnit in line with the existing SpeedUnit, here: https://github.com/victronenergy/venus-platform/blob/master/src/application.cpp#L192.

I don't have a strong opinion for either option. A bit far fetched, but I can image that altitude might also be used to indicate on which altitude the solar panels, irradiance sensor or some wind generator has been installed.

It is also a bit awkward that the gps'es don't appear under the device list, as I would have expected initially. But didn't touch that as I also don't want to make this PR bigger than it needs to be.

@@ -266,6 +286,30 @@ QtObject {
}
}

property VeQuickItem _altitudeUnit: VeQuickItem {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this currently supported by venus-platform?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That line 293 you mean? No, it isn't supported there yet. It could be that /Settings/System/Gps/AltitudeUnit is the better option.

See #1841 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

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

I expect we will wait for platform support before merging these changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initiated a change there. Lets indeed wait until that has been merged before continuing this PR.

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