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

Ambient Weather - Incorrect assignment of temperature data from extra modules to outdoor temperature #22

Open
dbusch78 opened this issue Jul 15, 2023 · 1 comment
Labels
[Feature] Extensibility and APIs All things related to new APIS, devices ... first time contributor Issue or PR opened by a first-time contributor to the repository [Needs] Dev [Type] Bug Something isn't working
Milestone

Comments

@dbusch78
Copy link

Hello there,

Congratulations on taking over the project. I really appreciate the ability to display my weather station's data on my website. I was pleased to discover that you are on GitHub, providing a platform for users to contribute and report bugs. I'm in Information Security as one of my Jobs so I hope this is complete and helps identify the issues.

Description:

I've noticed an issue in the file includes/traits/AmbientPluginBaseClient.php. The temperature data from extra modules ('temp' . $i . 'f') is being assigned to the 'temperature' key in the $device array. This seems to be overwriting the outdoor temperature data, resulting in the temperature from extra modules being displayed as the outdoor temperature.

I am using the Ambient Weather Station model WS-5000. However, based on a brief review of the API documentation from Ambient Weather, it appears that the API output might not be dependent on the model. Ambient Weather seems to have standardized the outputs and simply omits data if it doesn't exist for a particular model. I've only had this weather station for a few weeks.

Steps to reproduce:

  1. Add the station to the WordPress plugin.
  2. Wait for the API to sync.
  3. Verify that the outdoor temperature displayed is incorrect.
  4. Remove the station from the WordPress plugin.
  5. Enable debug logs.
  6. Re-add the station to the WordPress plugin.
  7. Wait for the API to sync again.
  8. Verify that the outdoor temperature displayed is still incorrect.
  9. Go to the debug logs and retrieve the API log.
  10. In the API log, find the correct outdoor temperature field and the additional temperature and humidity sensor (in this case, a Pool/Hot Tub Temp sensor).
  11. Verify that the incorrect outdoor temperature displayed is actually the temperature from the additional temperature and humidity sensor.
  12. The issue occurs consistently every time these steps are performed.

Expected behavior:

The outdoor temperature and the temperatures from extra modules should be stored and displayed separately.

Actual behavior:

The temperature from extra modules is being displayed as the outdoor temperature.

Debug logs:

Here are the debug logs showing the API output with the correct values:

Array
(
    [0] => Array
        (
            [macAddress] => C4:5B:BE:5D:33:FE
            [lastData] => Array
                (
                    // ...
                    [tempf] => 68, // This is the correct outdoor temperature
                    // ...
                    [temp8f] => 100.9, // This is an additional module (Pool/Hot Tub Sensor with temperature only)
                    // ...
                )
            // ...
        )
)

In the above output, [tempf] => 68 is the correct outdoor temperature. However, [temp8f] => 100.9 is actually an additional module which is a Pool/Hot Tub Sensor with temperature only. The issue is that the temperature from this additional module is being displayed as the outdoor temperature.

Suggested fix:

Consider storing the temperature and humidity data from extra modules in separate keys in the $device array to avoid overwriting the outdoor temperature and humidity data. For example:

$device['temperature' . $i] = $device['temp' . $i . 'f'];
$device['humidity' . $i] = $device['humidity' . $i];

This would need to be done in the following section of code:

// NAModule9 - max 9 extra modules
for ($i = 0; $i < 9; $i++) {
    if (array_key_exists('temp' . $i . 'f', $device) || array_key_exists('humidity' . $i, $device)) {
        if (array_key_exists('temp' . $i . 'f', $device)) {
            $device['temperature'] = $device['temp' . $i . 'f'];
        }
        else {
            unset($device['temperature']);
        }
        if (array_key_exists('humidity' . $i, $device)) {
            $device['humidity'] = $device['humidity' . $i];
        }
        else {
            unset($device['humidity']);
        }
        // ...
    }
}

Please note that other parts of the codebase that interact with the $device array might need to be updated to handle these new keys.


Full w/o commentary Debug logs:

Here are the debug logs showing the API output with the correct values:

Array
(
    [0] => Array
        (
            [macAddress] => C4:5B:BE:5D:33:FE
            [lastData] => Array
                (
                    [dateutc] => 1689394440000
                    [tempinf] => 75.6
                    [battin] => 1
                    [humidityin] => 51
                    [baromrelin] => 28.955
                    [baromabsin] => 28.955
                    [tempf] => 68
                    [battout] => 1
                    [battrain] => 1
                    [humidity] => 96
                    [winddir] => 154
                    [winddir_avg10m] => 151
                    [windspeedmph] => 2.2
                    [windspdmph_avg10m] => 5.1
                    [windgustmph] => 6.7
                    [maxdailygust] => 28.4
                    [hourlyrainin] => 0
                    [eventrainin] => 0.24
                    [dailyrainin] => 0.24
                    [weeklyrainin] => 0.874
                    [monthlyrainin] => 3.823
                    [yearlyrainin] => 4
                    [lightning_day] => 144
                    [lightning_time] => 1689382933000
                    [lightning_distance] => 12.43
                    [batt_lightning] => 0
                    [solarradiation] => 0
                    [uv] => 0
                    [temp3f] => 7.7
                    [humidity3] => 73
                    [temp4f] => -4.2
                    [humidity4] => 58
                    [temp5f] => 35.8
                    [humidity5] => 56
                    [temp6f] => 0.1
                    [humidity6] => 54
                    [temp8f] => 100.9
                    [batt3] => 1
                    [batt4] => 1
                    [batt5] => 1
                    [batt6] => 1
                    [batt7] => 1
                    [batt8] => 1
                    [pm25] => 11
                    [pm25_24h] => 23.1
                    [aqi_pm25] => 46
                    [aqi_pm25_24h] => 74
                    [batt_25] => 1
                    [batt_co2] => 1
                    [feelsLike] => 68
                    [dewPoint] => 66.81
                    [feelsLike3] => 7.7
                    [dewPoint3] => 0.9
                    [feelsLike4] => -4.2
                    [dewPoint4] => -15.2
                    [feelsLike5] => 35.8
                    [dewPoint5] => 21.7
                    [feelsLike6] => 0.1
                    [dewPoint6] => -12.6
                    [feelsLikein] => 75.3
                    [dewPointin] => 56.2
                    [lastRain] => 2023-07-15T03:52:00.000Z
                    [lightning_hour] => 0
                    [tz] => America/Chicago
                    [date] => 2023-07-15T04:14:00.000Z
                )

            [info] => Array
                (
                    [name] => Main Farm Weather Station
                    [coords] => Array
                        (
                            [coords] => Array
                                (
                                    [lat] => 41.096889
                                    [lon] => -89.999745
                                )

                            [address] => 2220 Golf Course Rd, La Fayette, IL 61449, USA
                            [location] => La Fayette
                            [elevation] => 228.0929107666
                            [geo] => Array
                                (
                                    [type] => Point
                                    [coordinates] => Array
                                        (
                                            [0] => -89.999745
                                            [1] => 41.096889
                                        )

                                )

                        )

                )

        )

)

@jaz-on
Copy link
Member

jaz-on commented Aug 1, 2023

Hello @dbusch78 thanks for this very detailed issue! <3
Your feedback is noted, as soon as I've got some time to test it, I'll get back to you on this.
I'm on holiday until ~September, I'm replying as I had to fix an urgent situation on #23.

Meanwhile, since you seem to have some ideas on how to tackle this issue, would you be willing to submit a PR?
I think we could iterate on your proposal quicker. What do you think?

@jaz-on jaz-on self-assigned this Aug 3, 2023
@jaz-on jaz-on added this to the 3.9 milestone Aug 3, 2023
@jaz-on jaz-on added [Type] Bug Something isn't working [Feature] Extensibility and APIs All things related to new APIS, devices ... [Needs] Dev first time contributor Issue or PR opened by a first-time contributor to the repository labels Aug 3, 2023
@jaz-on jaz-on removed their assignment Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility and APIs All things related to new APIS, devices ... first time contributor Issue or PR opened by a first-time contributor to the repository [Needs] Dev [Type] Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants