fix: Display all implemented sensors in SensorActivity #2584 #2589

samruddhi-Rahegaonkar · 2024-12-18T08:19:03Z

Updated the sensor addresses to be unique in the SensorActivity class, resolving conflicts where multiple sensors were assigned the same address (e.g., 0x39 for APDS9960 and TSL2561).
Refactored the sensor initialization to ensure accurate mapping between sensor addresses and their names, allowing all implemented sensors to be correctly shown during the sensor scan.

Checklist:

No hard coding: I have used resources from strings.xml, dimens.xml, and colors.xml without hard coding any value.

No end of file edits: No modifications done at the end of resource files strings.xml, dimens.xml, or colors.xml.

Code reformatting: I have reformatted code and fixed indentation in every file included in this pull request.

No extra space: My code does not contain any extra lines or extra spaces than the ones that are necessary.

Summary by Sourcery

Fix sensor address conflicts in SensorActivity by assigning unique addresses to each sensor and refactor initialization for accurate mapping. Update the IndicatorSeekBar dependency version in the build configuration.

Bug Fixes:

Resolve conflicts in sensor addresses by assigning unique addresses to each sensor in the SensorActivity class.

Enhancements:

Refactor sensor initialization to ensure accurate mapping between sensor addresses and their names.

Build:

Update the version of the IndicatorSeekBar dependency in the build configuration.

sourcery-ai · 2024-12-18T08:19:08Z

Reviewer's Guide by Sourcery

This PR fixes sensor display issues in SensorActivity by updating I2C addresses to ensure uniqueness for all sensors and cleaning up build dependencies. The main changes involve reassigning addresses for CCS811 and APDS9960 sensors to prevent conflicts, and updating Gradle dependencies.

No diagrams generated as the changes look simple and do not need a visual representation.

File-Level Changes

Change	Details	Files
Updated I2C sensor addresses to resolve conflicts	Added comments to clarify the purpose of each sensor address Changed CCS811 sensor address from 0x5A to 0x3A to avoid conflict with MLX90614 Changed APDS9960 sensor address from 0x39 to 0x3B to avoid conflict with TSL2561	`app/src/main/java/io/pslab/activity/SensorActivity.java`
Gradle build configuration updates	Updated IndicatorSeekBar dependency version format Downgraded Android application plugin from 8.7.3 to 8.5.1 Removed duplicate IndicatorSeekBar implementation	`app/build.gradle.kts` `build.gradle.kts`

Possibly linked issues

fix: Smoothed the data line and changed some graph properties in Lux Meter instrument #1234: The PR resolves the issue by ensuring unique sensor addresses, fixing the missing sensors problem.

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time. You can also use
this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

sourcery-ai

Hey @samruddhi-Rahegaonkar - I've reviewed your changes - here's some feedback:

Overall Comments:

Please verify that the new I2C addresses (0x3A for CCS811 and 0x3B for APDS9960) are valid and supported by these sensors. I2C addresses are typically hardware-defined and cannot be arbitrarily changed in software.

Here's what I looked at during the review

🟢 General issues: all looks good
🟢 Security: all looks good
🟢 Testing: all looks good
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

github-actions · 2024-12-18T08:23:39Z

Build successful. APKs to test: https://github.com/fossasia/pslab-android/actions/runs/12388981148/artifacts/2336329720

AsCress · 2024-12-21T11:21:08Z

Hello @samruddhi-Rahegaonkar ! Welcome here!
Thanks for your work on this issue !
Let me provide some context on those duplicate numbers as asked by @marcnause as well in #2584.
These numbers are actually unique addresses for each sensor, which is something which we can't decide upon, but rather is fixed by the sensor manufacturer itself.
This unique address can be found in the datasheet of each respective sensor. The I2C bus identifies anything connected on it using this address.
Since this address is a 7-bit (or 10-bit number), only 128 (or 1024) addresses for I2C sensors are possible, which makes it clear that there would definitely be duplications (there are way more sensors in the world).
However, this duplication would cause a problem only if two sensors having the same address are connected to the I2C bus (which isn't gonna be the case most of the times with the PSLab).
Therefore, we can safely keep the duplications and add those sensors. We can however inform the user, each time the PSLab detects a sensor connection which can possibly have a duplicacy. In such a either one of these sensors would be operable (if more than one with the same address are connected).

AsCress · 2024-12-21T11:25:21Z

As for this pull-request, it doesn't actually solve the mentioned issue of displaying the sensors which arn't listed in the SensorActivity.
@samruddhi-Rahegaonkar Possibly you can add those in the list, without changing any of the addresses.
I'll mark the necessary changes in a review to make it easy for you.

AsCress · 2024-12-21T11:26:09Z

app/build.gradle.kts

@@ -83,7 +83,6 @@ dependencies {
    implementation("com.github.mirrajabi:search-dialog:1.2.4")
    implementation(files("../libs/croller-release.aar"))
    implementation("com.github.BeppiMenozzi:Knob:1.9.0")
-    implementation("com.github.warkiz:IndicatorSeekBar:v2.1.1")


Some changes which arn't required. You can just rollback build.gradle.kts.

AsCress · 2024-12-21T11:27:56Z

app/src/main/java/io/pslab/activity/SensorActivity.java

+        sensorAddr.put(0x39, "TSL2561");  // Unique address for TSL2561
+        sensorAddr.put(0x69, "MPU925x");  // Unique address for MPU925x
+        sensorAddr.put(0x29, "VL53L0X");  // Unique address for VL53L0X
+        sensorAddr.put(0x3A, "CCS811");  // New unique address for CCS811


As mentioned earlier, changing the address would make the sensor inoperable. You can retain the same address.

AsCress · 2024-12-21T11:28:05Z

app/src/main/java/io/pslab/activity/SensorActivity.java

+        sensorAddr.put(0x69, "MPU925x");  // Unique address for MPU925x
+        sensorAddr.put(0x29, "VL53L0X");  // Unique address for VL53L0X
+        sensorAddr.put(0x3A, "CCS811");  // New unique address for CCS811
+        sensorAddr.put(0x3B, "APDS9960"); // New unique address for APDS9960


Same for this.

AsCress · 2024-12-21T11:28:39Z

build.gradle.kts

@@ -11,5 +11,5 @@ buildscript {
 }

 plugins {
-    id("com.android.application") version "8.7.3" apply false


Again, some changes that arn't required. You can rollback these.

Okay @AsCress Thanks for Explaining me. I will do the needful !

@AsCress Other than the SensorActivity the changes I have made because I was unable to run the application without making those changes. But after testing I will rollback it.

@AsCress to resolve this issue as you have said, I need to make changes in the method "PopulateSensors" to handle the cases where both having same addresses.
should I ?

@samruddhi-Rahegaonkar The problem here is with the data structure that we are using to store the sensor addresses.
A Map in Java doesn't allow duplicate keys to be inserted. When we try to do that in lines 86 and 87 of SensorActivity.java, it overwrites the previous entries, and hence the previous two sensors (MLX90614 and TSL2561) actually never get inserted in the Map.
The idea here would be to migrate to a different data structure which would allow storage of duplicate entries, and then using that in PopulateSensors to display them.

@AsCress Instead of Map<Integer, String>, we can use a Map<Integer, List<String>> to associate multiple sensor names with the same address.

@samruddhi-Rahegaonkar Yep, that's one way. Go ahead and maybe try to implement whatever you find best in this PR.

…nsor-activity

Fix: Display all implemented sensors in SensorActivity fossasia#2584

1c27f92

sourcery-ai bot reviewed Dec 18, 2024

View reviewed changes

samruddhi-Rahegaonkar mentioned this pull request Dec 18, 2024

Not all sensors which have been implemented are shown in SensorActivity #2584

Open

marcnause changed the title ~~Fix: Display all implemented sensors in SensorActivity #2584~~ fix: Display all implemented sensors in SensorActivity #2584 Dec 21, 2024

AsCress suggested changes Dec 21, 2024

View reviewed changes

samruddhi-Rahegaonkar added 3 commits December 25, 2024 10:46

fix: Display all implemented sensors in SensorActivity

4ff8ce3

Fixed display All Sensors

584f6c9

Merge remote-tracking branch 'origin/fix-sensor-activity' into fix-se…

9321d17

…nsor-activity

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Display all implemented sensors in SensorActivity #2584 #2589

fix: Display all implemented sensors in SensorActivity #2584 #2589

samruddhi-Rahegaonkar commented Dec 18, 2024 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Dec 18, 2024 •

edited

Loading

Interacting with Sourcery

Customizing Your Experience

Getting Help

sourcery-ai bot left a comment

github-actions bot commented Dec 18, 2024

AsCress commented Dec 21, 2024 •

edited

Loading

AsCress commented Dec 21, 2024

AsCress Dec 21, 2024

AsCress Dec 21, 2024

AsCress Dec 21, 2024

AsCress Dec 21, 2024

samruddhi-Rahegaonkar Dec 22, 2024

samruddhi-Rahegaonkar Dec 22, 2024

samruddhi-Rahegaonkar Dec 23, 2024

AsCress Dec 23, 2024

samruddhi-Rahegaonkar Dec 24, 2024

AsCress Dec 24, 2024

fix: Display all implemented sensors in SensorActivity #2584 #2589

Are you sure you want to change the base?

fix: Display all implemented sensors in SensorActivity #2584 #2589

Conversation

samruddhi-Rahegaonkar commented Dec 18, 2024 • edited by sourcery-ai bot Loading

Summary by Sourcery

sourcery-ai bot commented Dec 18, 2024 • edited Loading

Reviewer's Guide by Sourcery

File-Level Changes

Possibly linked issues

Interacting with Sourcery

Customizing Your Experience

Getting Help

sourcery-ai bot left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 18, 2024

AsCress commented Dec 21, 2024 • edited Loading

AsCress commented Dec 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samruddhi-Rahegaonkar commented Dec 18, 2024 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Dec 18, 2024 •

edited

Loading

AsCress commented Dec 21, 2024 •

edited

Loading