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

Power settings #276

Closed
wants to merge 2 commits into
base: dev
from

Conversation

Projects
None yet
2 participants
@mpele
Contributor

mpele commented Sep 30, 2015

According to #273 it has been added field powerConditions in table Devices and in dialog DeviceDialog.

@vitalidze

This comment has been minimized.

Show comment
Hide comment
@vitalidze

vitalidze Sep 30, 2015

Owner

I will not merge this because:

  • it doesn't look finished: no logic where the value is applied, translation is confusing
  • I don't have an understanding on how it should work. What should I enter in the text field?
  • I still insist that since 'power' is a sensor then it's settings should be defined for the sensor itself. Then you may set up a way to detect 'Idle' by using that type of sensor (like discussed in #273) in device settings.
Owner

vitalidze commented Sep 30, 2015

I will not merge this because:

  • it doesn't look finished: no logic where the value is applied, translation is confusing
  • I don't have an understanding on how it should work. What should I enter in the text field?
  • I still insist that since 'power' is a sensor then it's settings should be defined for the sensor itself. Then you may set up a way to detect 'Idle' by using that type of sensor (like discussed in #273) in device settings.
@mpele

This comment has been minimized.

Show comment
Hide comment
@mpele

mpele Sep 30, 2015

Contributor

The logic is applied in the branch mdev - for generating detailed reports
and representing locations on map where were stops. Right now in the code
it is used constant string ("power":1) to check its presents in the
field Other and idea is to generalize it for user input. In my case, I
would enter "power":1 or "io240":1 in new field.

I do not understand on what you insist in third bullet.

If this pullrequest is accepted next step would be merging the mdev in your
HEAD. There is things that should be done (right now there is missing
dialog for reports) but it looks like that it is ready for merging with
main branch.

On Wed, Sep 30, 2015 at 8:17 AM, Vitaly Litvak notifications@github.com
wrote:

I will not merge this because:

  • it doesn't look finished: no logic where the value is applied,
    translation is confusing
  • I don't have an understanding on how it should work. What should I
    enter in the text field?
  • I still insist that since 'power' is a sensor then it's settings
    should be defined for the sensor itself. Then you may set up a way to
    detect 'Idle' by using that type of sensor (like discussed in #273
    #273) in device
    settings.


Reply to this email directly or view it on GitHub
#276 (comment)
.

Contributor

mpele commented Sep 30, 2015

The logic is applied in the branch mdev - for generating detailed reports
and representing locations on map where were stops. Right now in the code
it is used constant string ("power":1) to check its presents in the
field Other and idea is to generalize it for user input. In my case, I
would enter "power":1 or "io240":1 in new field.

I do not understand on what you insist in third bullet.

If this pullrequest is accepted next step would be merging the mdev in your
HEAD. There is things that should be done (right now there is missing
dialog for reports) but it looks like that it is ready for merging with
main branch.

On Wed, Sep 30, 2015 at 8:17 AM, Vitaly Litvak notifications@github.com
wrote:

I will not merge this because:

  • it doesn't look finished: no logic where the value is applied,
    translation is confusing
  • I don't have an understanding on how it should work. What should I
    enter in the text field?
  • I still insist that since 'power' is a sensor then it's settings
    should be defined for the sensor itself. Then you may set up a way to
    detect 'Idle' by using that type of sensor (like discussed in #273
    #273) in device
    settings.


Reply to this email directly or view it on GitHub
#276 (comment)
.

@vitalidze

This comment has been minimized.

Show comment
Hide comment
@vitalidze

vitalidze Sep 30, 2015

Owner

I don't like the idea for user to mess with XML/JSON values.

What I meant is that there are already sensors settings, where you define their names and intervals. 'Power' and 'io240' are both sensors. So I believe you should define their possible values (like 0/1) and types (like 'ignition' or 'battery') in the same place, i.e. on the same tab. You can also copy these settings between devices if needed. Then if you want to use it for stops (or trips) detection you should define it at the same place where you define the 'idle speed limit' setting. I think this way it will be more user-friendly.

Owner

vitalidze commented Sep 30, 2015

I don't like the idea for user to mess with XML/JSON values.

What I meant is that there are already sensors settings, where you define their names and intervals. 'Power' and 'io240' are both sensors. So I believe you should define their possible values (like 0/1) and types (like 'ignition' or 'battery') in the same place, i.e. on the same tab. You can also copy these settings between devices if needed. Then if you want to use it for stops (or trips) detection you should define it at the same place where you define the 'idle speed limit' setting. I think this way it will be more user-friendly.

@mpele

This comment has been minimized.

Show comment
Hide comment
@mpele

mpele Oct 1, 2015

Contributor

OK. I though that, at this stage of iteration, is more important functionality than to be user-friendly.

Contributor

mpele commented Oct 1, 2015

OK. I though that, at this stage of iteration, is more important functionality than to be user-friendly.

@mpele mpele closed this Oct 1, 2015

@vitalidze

This comment has been minimized.

Show comment
Hide comment
@vitalidze

vitalidze Oct 1, 2015

Owner

I don't like the idea to re-implement it later, so from my point of view this should be done using the right approach from the start. Maybe the UI should lack, or other functionality should lack, but we should look into future while making these decisions. I don't see any reason why it can't be done as a setting for sensor, it's not so hard to implement, just a little bit more work required.

Anyway, in near future I am planning to start working myself on #210 and the reports will be close to the version described here. I will add sensor types and make other necessary changes during implementation.

Owner

vitalidze commented Oct 1, 2015

I don't like the idea to re-implement it later, so from my point of view this should be done using the right approach from the start. Maybe the UI should lack, or other functionality should lack, but we should look into future while making these decisions. I don't see any reason why it can't be done as a setting for sensor, it's not so hard to implement, just a little bit more work required.

Anyway, in near future I am planning to start working myself on #210 and the reports will be close to the version described here. I will add sensor types and make other necessary changes during implementation.

@mpele mpele deleted the mpele:powerSettings branch Feb 3, 2016

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