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

Add voltage and amperage scale calibration #1439

Merged
merged 1 commit into from
May 28, 2019
Merged

Conversation

IvoFPV
Copy link
Contributor

@IvoFPV IvoFPV commented May 16, 2019

Adds voltage meter scale calibration by setting the actual battery voltage.
Setting the parameter to anything but zero will adjust scale automatically after saving.

@McGiverGim
Copy link
Member

To me is a good idea, but I don't see it in the middle of the battery tab. I think is better to have a little modal window, that shows this option. Can you look for example to the OSD tab to see how the font manager window is being done to see if it is possible to do something in this way? @mikeller, what do you think?

@IvoFPV
Copy link
Contributor Author

IvoFPV commented May 17, 2019

I thought about making a separate window like that, but to me it is unnecessary for just one setting and a save button. If there were more settings related to this then it would be a good idea.

@IvoFPV IvoFPV changed the title Add voltage scale calibration Add voltage and amperage scale calibration May 17, 2019
Copy link
Member

@mikeller mikeller left a comment

Choose a reason for hiding this comment

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

I think that this is a good idea, but I agree with @McGiverGim that the current UX is confusing, and that this should better be done with a modal dialog: Add a 'calibrate' button, pop open a dialog with the help text and the input and a 'Calibrate' button, and update the scale when a user clicks 'Calibrate'.
Note that the amperage popup should also tell the user that the offset needs to be measured / entered before using this calibration, or else it will not work correctly.

src/js/tabs/power.js Outdated Show resolved Hide resolved
src/js/tabs/power.js Outdated Show resolved Hide resolved
src/tabs/power.html Outdated Show resolved Hide resolved
@IvoFPV
Copy link
Contributor Author

IvoFPV commented May 18, 2019

I just added the modal dialog with its button near Save, to me it looks okay. Maybe I could make the note a bit better to understand.

Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

Thanks for the modal window! This looks far better.

src/tabs/power.html Outdated Show resolved Hide resolved
McGiverGim
McGiverGim previously approved these changes May 18, 2019
Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

Good job! At least it's a good first version 😁

locales/en/messages.json Outdated Show resolved Hide resolved
locales/en/messages.json Outdated Show resolved Hide resolved
src/js/tabs/power.js Outdated Show resolved Hide resolved
src/js/tabs/power.js Outdated Show resolved Hide resolved
@IvoFPV
Copy link
Contributor Author

IvoFPV commented May 23, 2019

I added the confirmation dialog showing the new calibrated scales with option to apply or discard them. Applying them will only save the new scale values and not other settings that user might have changed in power tab.

locales/en/messages.json Outdated Show resolved Hide resolved
src/js/tabs/power.js Outdated Show resolved Hide resolved
@IvoFPV
Copy link
Contributor Author

IvoFPV commented May 25, 2019

I totally removed saving from the calibration and now when they are applied they only get set as an input value and must be saved by clicking save on power tab.

@mikeller
Copy link
Member

This is starting to look good now.
Two things:

  • the 'Calibrate' button is currently only shown if both voltage and amperage source support calibration:
    image
    Maybe show it if only one supports calibration, and only show the corresponding input field?
  • since calibration will not work if the detected voltage or current is not 0, maybe hide the input field for sensors that are reading 0 when opening the dialog as well?

@IvoFPV
Copy link
Contributor Author

IvoFPV commented May 26, 2019

Calibrate button should be there if there is at least one scale that can be calibrated. When you change voltage or amperage source it gets hidden because that source setting is not saved. After saving it will appear again if any of sources support scaling.
I will make it to only be possible to calibrate when the readings are not 0, meaning the battery is not plugged in or sources are not set right.

@IvoFPV
Copy link
Contributor Author

IvoFPV commented May 26, 2019

Now I changed how showing/hiding works. Calibrate button is always shown in bottom bar if there is a scale to calibrate, but when opened it shows warnings if battery not plugged in or sources have changed but haven't been saved. It should work with just voltage or amperage or both. I added checks to remove input if voltage/amperage are less than 0.1, meaning reading is invalid or meter source is incorrect.

Copy link
Member

@mikeller mikeller left a comment

Choose a reason for hiding this comment

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

This is working fine now, good work!

There is one stray whitespace, and once this is fixed it will need to be rebased / squashed into one commit.

src/js/tabs/power.js Outdated Show resolved Hide resolved
@mikeller mikeller added this to the 10.6.0 milestone May 27, 2019
@IvoFPV
Copy link
Contributor Author

IvoFPV commented May 27, 2019

I fixed the whitespace and rebased it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants