Skip to content

Conversation

@ho-dor
Copy link

@ho-dor ho-dor commented May 28, 2019

Fixes #1750 #1752

Changes: Added backend code for thermometer .

Screenshot/s for the changes:
Screenshot_20190529-185838_PSLab

Checklist: [Please tick following check boxes with [x] if the respective task is completed]

  • I have used resources from strings.xml, dimens.xml and colors.xml without hard-coding them
  • No modifications done at the end of resource files strings.xml, dimens.xml or colors.xml
  • I have reformatted code in every file included in this PR [CTRL+ALT+L]
  • My code does not contain any extra lines or extra spaces
  • I have requested reviews from other members

APK for testing:
app-debug.zip

@ho-dor ho-dor force-pushed the development branch 2 times, most recently from e2c6c73 to f3fe57e Compare May 28, 2019 21:31
@ho-dor ho-dor mentioned this pull request May 28, 2019
5 tasks
Copy link
Collaborator

@CloudyPadmal CloudyPadmal left a comment

Choose a reason for hiding this comment

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

Make sure all the wordings have been replaced correctly when copying another class as whole. It might lead to errors. Only make the PR when it is clean and ready. Fixing these kind of stuff is not part of review.

android:padding="15dp"
app:sv_speedTextColor="@color/black"
app:sv_speedTextPosition="BOTTOM_CENTER"
app:sv_speedTextSize="@dimen/lux_display_font_size"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you make separate variables for thermometer replacing the luxs?

Copy link
Author

Choose a reason for hiding this comment

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

No . I kept it the same knowigly because the dimen value are going to be same eventually in UI but I will change it if that is better

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, all needs to be changed. The real reason why we are using resources instead of hard-coded values is that, it will be easier and practical to make modifications from one place. Putting luxmeter dimensions is in fact similar to hard-coding.

<string name="thermometer_bottom_sheet_text">Thermometer instrument is used to measure ambient temprature. It can be measured using inbuilt ambient temprature sensor or through SHT21.</string>
<string name="thermometer_bottom_sheet_desc">Thermometer instrument is used to measure ambient temprature. It can be measured using inbuilt ambient temprature sensor or through SHT21.</string>
<string name="no_thermometer_sensor">Device does not have Thermometer Sensor</string>
<string name="thermo_unit">F</string>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll use Celcius

Copy link
Collaborator

Choose a reason for hiding this comment

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

And let user select which unit want from configurations

Copy link
Author

Choose a reason for hiding this comment

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

Yes , I am going to add that functionality . I just wanted to confirm it works or not till here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was referring to units here. Throughout the app we are using Metric units, not imperial


count++;
sum += entry.getY();
statMean.setText(String.format(Locale.getDefault(), PSLabSensor.LUXMETER_DATA_FORMAT, (sum / count)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

All these Lux stuff need to be thermo related

Copy link
Member

@neel1998 neel1998 left a comment

Choose a reason for hiding this comment

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

Do we need to have max value to be 10000 F? It seems too high

@CloudyPadmal
Copy link
Collaborator

@neel1998 definitely no. @ho-dor check the datasheet of the sensor and set the max value to the maximum temperature value in that.

@CloudyPadmal
Copy link
Collaborator

Besides, i think a gauge is not a good way to represent data in a thermometer. We can implement some color bar instead. Please look into this @ho-dor . Thanks.

@ho-dor
Copy link
Author

ho-dor commented May 29, 2019

Besides, i think a gauge is not a good way to represent data in a thermometer. We can implement some color bar instead. Please look into this @ho-dor . Thanks.

How about we change the background of gauge as the temp changes, like make it green for low temp and red for higher temp ?

@CloudyPadmal
Copy link
Collaborator

Besides, i think a gauge is not a good way to represent data in a thermometer. We can implement some color bar instead. Please look into this @ho-dor . Thanks.

How about we change the background of gauge as the temp changes, like make it green for low temp and red for higher temp ?

Ok for now. Blue for low temps and red for high temps

@ho-dor
Copy link
Author

ho-dor commented May 29, 2019

@neel1998 @CloudyPadmal please check

import android.os.Handler;
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;
import android.support.v14.preference.PreferenceFragment;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Codacy complains this as unused

@CloudyPadmal
Copy link
Collaborator

Rest looks ok. We can progress from here

@neel1998
Copy link
Member

Looks ok to me as well

@ho-dor
Copy link
Author

ho-dor commented May 30, 2019

codacy resolved

@ho-dor
Copy link
Author

ho-dor commented May 30, 2019

Once this gets merged , I will make a PR for #1756. @CloudyPadmal @neel1998

@ho-dor
Copy link
Author

ho-dor commented May 30, 2019

I have changed the sensor to SHT21 as that is more suitable for this purpose.

@CloudyPadmal CloudyPadmal requested a review from neel1998 May 30, 2019 08:52
@neel1998
Copy link
Member

@ho-dor there is still one unused import in codacy. Remove it in this PR or remember to remove in next one. Also can you send the latest apk once to test

@ho-dor
Copy link
Author

ho-dor commented May 30, 2019

@neel1998 check

@neel1998
Copy link
Member

Can you comment the apk file here?

@ho-dor
Copy link
Author

ho-dor commented May 30, 2019

Can you comment the apk file here?

The apk file isn't supported to upload here . I have added the latest zip of apk in PR

@neel1998
Copy link
Member

The graph is acting weirdly. Please look at the gif. It goes blank after a while. Please look at the barometer graph. It seems to be working okay
20190530_155715

@ho-dor
Copy link
Author

ho-dor commented May 30, 2019

I have increased the range on x-axis and the graph is more like lux meter. I am unable to understand the graph of baro meter as the max value that pressure can reach is 2 according to the range but the graph only shows till 0.45 atm on y-axis.

@neel1998
Copy link
Member

This has nothing to do with range, the x - axis should increment every second, which is not happening in the thermometer, also the graph going blank after a while is also not an issue of range

@ho-dor
Copy link
Author

ho-dor commented May 30, 2019

This has nothing to do with range, the x - axis should increment every second, which is not happening in the thermometer, also the graph going blank after a while is also not an issue of range

I have set the graph updation period to 100 ms in place of 1000 ms that is why it is getting updated quick , but the graph going blank is because of the x-axis range that I have set to 800 now , if you will see , now the graph continues till 800 s earlier it was till 80 s.

@neel1998
Copy link
Member

So have you changed to 1000 ms?

@ho-dor
Copy link
Author

ho-dor commented May 30, 2019

Yes , check the updated apk

@neel1998
Copy link
Member

looks good now

@CloudyPadmal CloudyPadmal merged commit a9701ac into fossasia:development May 30, 2019
neel1998 pushed a commit to neel1998/pslab-android that referenced this pull request Jul 30, 2019
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.

Adding the working of thermometer

4 participants