samemory / homebridge-eufy-security

Work in progress
Apache License 2.0
97 stars 14 forks source link

[Bug]: Original Floodlight Cam - "Wrong Return Value" in log #230

Closed fullphat closed 2 years ago

fullphat commented 3 years ago

What happened?

I have the original Floodlight Cam (T8420) and my HomeBridge log is full of the following:

[9/7/2021, 11:28:44 AM] [EufySecurity] Garden handleEnableGet Wrong return value [9/7/2021, 11:28:44 AM] [EufySecurity] Garden handleMotionOnGet Wrong return value

Anything I can do to help troubleshoot the issue?

Thanks!

Device Type

Floodlight (Supported)

Plugin Version

v1.0.x (Supported)

HomeBridge Version

v1.3.x (Supported)

NodeJS Version

v14 (Supported)

Operating System

Mac OS X

Relevant log output

[9/7/2021, 11:28:44 AM] [EufySecurity] Garden handleEnableGet Wrong return value
[9/7/2021, 11:28:44 AM] [EufySecurity] Garden handleMotionOnGet Wrong return value
github-actions[bot] commented 3 years ago

Did you check this Common Issues pages ?

fullphat commented 3 years ago

Yes, but apologies if I've missed something. I did search Google and GitHub too. The issues page does confirm this camera is supported:

Floodlight (T8420)

Thanks

fullphat commented 3 years ago

Any update on this? It seems that the motion and camera enabled switches are showing the wrong value (always display as 'off') due to this issue.

github-actions[bot] commented 3 years ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

fullphat commented 3 years ago

Could I get an update please?

gdotp01 commented 3 years ago

I’ve been using the plugin successfully for 6 mobpnths with two batter doorbells. Thanks

I tried yesterday to add the t8420 floodlight camera and although it shows up in HomeKit as a camera the buttons don’t seem to do anything and the motion sensor does not trip.

I’ve looked at the “common issues” section and tried various resolutions but none of these seem to get it working

any ideas?

mariusstrom commented 2 years ago

Also seeing the same issue.

smitty078 commented 2 years ago

also seeing this issue

fullphat commented 2 years ago

@lenoxys is this an issue with the client library or with this plugin? I've had a look in CameraAccessory.ts and I'm guessing this is the relevant code:

const currentValue = this.Camera.getPropertyValue(PropertyName.DeviceEnabled);

"this.Camera" comes from eufy-security-client so I'm guess the issue lies there?

smitty078 commented 2 years ago

I’ve been working on isolating this for the past 24 hours - more to post soon - but this definitely an issue in the client library. I don’t have access to a camera with an old firmware but as a guess it appears that Eufy made some changes in a recent firmware update that broke the library. I poked a red with the library code and either a) Eufy changed something on their end or b) the library is frankly a disaster (it isn’t)

smitty078 commented 2 years ago

@lenoxys is this an issue with the client library or with this plugin? I've had a look in CameraAccessory.ts and I'm guessing this is the relevant code:

const currentValue = this.Camera.getPropertyValue(PropertyName.DeviceEnabled);

"this.Camera" comes from eufy-security-client so I'm guess the issue lies there?

My opinion is that you nailed it with your report here: https://github.com/bropat/eufy-security-client/issues/72#issue-1067380691

fullphat commented 2 years ago

Given the above, I propose to close this ticket and continue the conversation in the eufy-security-client repository instead. Makes sense?

fullphat commented 2 years ago

Most functions now seem to work, although the external floodlights (which do work in the 1.1 beta) still don't work in 1.0.8.

Update: I've been able to make the floodlights work by pasting the appropriate code from the beta into 1.0.8 - it doesn't properly read the current light state, but it's good for what I need right now. 😊

schliemann commented 2 years ago

@fullphat If there are code changes that would make sense to move from the beta to master then you are welcome to make a PR.

fullphat commented 2 years ago

Happy to, but I've never done one before and I'm not sure how to open one (when I try, I need to compare repos, and I'm not sure which I should pick). I've now got it working (it's basically a small number of changes to cameraaccessory.js) so if someone could guide me through the process, I'll get it done ASAP.

schliemann commented 2 years ago

@fullphat Do you have other changes than the vcodec streaming error?

fullphat commented 2 years ago

The base station / alarm doesn't work correctly at the moment although I can't figure out why. I have the camera set to 'Schedule' mode in the Eufy app, but in the Home app it just says "Arming...". In the log file I see:

[1/8/2022, 9:15:45 AM] [homebridge-eufy-security] This plugin generated a warning from the characteristic 'Security System Current State': characteristic was supplied illegal value: number -1 exceeded minimum of 0. See https://git.io/JtMGR for more info.

Other than that, I think everything else is now working ok!

schliemann commented 2 years ago

The base station / alarm doesn't work correctly at the moment although I can't figure out why. I have the camera set to 'Schedule' mode in the Eufy app, but in the Home app it just says "Arming...". In the log file I see:

[1/8/2022, 9:15:45 AM] [homebridge-eufy-security] This plugin generated a warning from the characteristic 'Security System Current State': characteristic was supplied illegal value: number -1 exceeded minimum of 0. See https://git.io/JtMGR for more info.

Other than that, I think everything else is now working ok!

Odd. Can you share your log and security part of the config?

https://user-images.githubusercontent.com/10818044/148643183-14072be4-c0d8-4783-b470-b3b90021c20a.MOV

fullphat commented 2 years ago

Here's a bit more from the log (I added the 'warning' logging):

[1/8/2022, 12:02:51 PM] [EufySecurity-1.1.1-beta.14] DEBUG: Ext Garden HL GET StationCurrentMode: { value: -1, timestamp: 0 }
[1/8/2022, 12:02:51 PM] [EufySecurity-1.1.1-beta.14] INFO:  WARNING -1

Given the timestamp property is zero, does that mean the T8420 is ignoring this property?

HomeKit to Eufy mapping in my config is:

"enableCamera": true,
            "pollingIntervalMinutes": 1,
            "hkHome": 1,
            "hkAway": 0,
            "hkNight": 2,
            "hkOff": 63,
            "enableDetailedLogging": 1,
schliemann commented 2 years ago

Ah. It's the alarm level of the floodlight? Or am I mistanken?

fullphat commented 2 years ago

Ah. It's the alarm level of the floodlight? Or am I mistanken?

Yes, that's right. The T8420 connects directly to my WiFi so it doesn't need a separate base station. I can control the arm mode in the Eufy app and it seems to work ok if I change the arm mode using HomeKit. For some reason though it sometimes gets "stuck" and then starts reporting -1 as it's status.

Just done some tests:

Set to "home" via Home app - works Set to "away" via Home app - works Set to "night" via Home app - gets stuck "arming" in Home app; eEufy app shows "schedule" Set to "off" via Home app - works

schliemann commented 2 years ago

Is it acting as a station in homekit? Didn't think the client library would return it as a station.

fullphat commented 2 years ago

Is it acting as a station in homekit?

If you mean if it's acting as an Alarm System, then yes! 😊

image

schliemann commented 2 years ago

Haha. Yes you are right. We create the security Accessory from the stations that eufy security client returns. Might be because the modes are handled differently for floodlights.

schliemann commented 2 years ago

Your problem is probably that schedule = 2 doesn't exist on alarmmode

7F161CDA-FB4D-49C5-BCA6-668B62DB565E

schliemann commented 2 years ago

We could extend this so you would need a secondary config section for alarm devices such as the floodlight?

Dunno. Whats your thoughts?

fullphat commented 2 years ago

More tests:

Set to "Away" in Eufy app - gets stuck "arming" in Home app Set to "Home" in Eufy app - works Set to "Disarm" in Eufy app - Home app gets stuck "disarming" Set to "Schedule" in Eufy app - gets stuck "arming" in Home app

Seems that the plugin works better sending status changes to the camera but not so well retrieving status changes from it.

schliemann commented 2 years ago

I think we need your logs with insane logging level. And then for you to write in the logs when you did what and if it's from the eufy app or homekit.

I don't have a floodlight. So can't test.

fullphat commented 2 years ago

Ok, leave it with me - I'll up the debugging level and see what I can do.

fullphat commented 2 years ago

Made some progress by making some changes to stationAccessory.js.

Firstly, I've made onStationGuardModePushNotification set both the HomeKit target state and current state (not sure this is the right thing to do however) and have stopped onStationCurrentModePushNotification from doing anything:

    onStationGuardModePushNotification(station, guardMode) {
        this.platform.log.debug(this.accessory.displayName, 'ON SecurityGuardMode:', guardMode);
        this.platform.log.info('++++ onStationGuardModePushNotification ++++');
        const homekitCurrentMode = this.convertEufytoHK(guardMode);
        this.service
            .getCharacteristic(this.characteristic.SecuritySystemTargetState)
            .updateValue(homekitCurrentMode);

          //mine
          this.service
            .getCharacteristic(this.characteristic.SecuritySystemCurrentState)
            .updateValue(homekitCurrentMode);
          //mine
    }

    onStationCurrentModePushNotification(station, currentMode) {
        this.platform.log.debug(this.accessory.displayName, 'ON SecuritySystemCurrentState:', currentMode);
        this.platform.log.info('++++ onStationCurrentModePushNotification ++++');
        // const homekitCurrentMode = this.convertEufytoHK(currentMode);
        //
        // this.platform.log.info('SecuritySystemCurrentState translated to HomeKit mode:', homekitCurrentMode);
        //
        // this.service
        //     .getCharacteristic(this.characteristic.SecuritySystemCurrentState)
        //     .updateValue(homekitCurrentMode);
    }

And changed handleSecuritySystemCurrentStateGet to use PropertyName.StationGuardMode:

    async handleSecuritySystemCurrentStateGet() {
        if (this.alarm_triggered) {
            return this.characteristic.SecuritySystemCurrentState.ALARM_TRIGGERED;
        }
        try {
            // const currentValue = this.eufyStation.getPropertyValue(eufy_security_client_1.PropertyName.StationCurrentMode);
            const currentValue = this.eufyStation.getPropertyValue(eufy_security_client_1.PropertyName.StationGuardMode);
            this.platform.log.info(this.accessory.displayName, 'GET StationGuardMode:', currentValue);

            if (currentValue.value == -1) {
              this.platform.log.info('WARNING: converting invalid value ', this.convertEufytoHK(currentValue.value), ' to 1');
              return 1;
            }

            return this.convertEufytoHK(currentValue.value);
        }
        catch (_a) {
            this.platform.log.error(this.accessory.displayName, 'handleSecuritySystemCurrentStateGet', 'Wrong return value');
            return false;
        }
    }

It's still a work-in-progress, but setting the alarm from the Home app now works successfully for all values (including 'Night', which sets my camera to Scheduled).

Will continue testing and refining.

schliemann commented 2 years ago

Can you make a fork of your own? That would make understanding your changes easier. I think there might be something that breaks in your changes. My approach would have been to determine the stationtype and then only alter where functionality differ.

fullphat commented 2 years ago

Can you make a fork of your own? That would make understanding your changes easier. I think there might be something that breaks in your changes. My approach would have been to determine the stationtype and then only alter where functionality differ.

Yes, makes sense. I assume there is a suitable helper function like is_T8420() or device_type_matches(CAMERA_T8420) I could use for this?

Will tidy up my code and get some proper comments into it and submit a PR. 👍

schliemann commented 2 years ago

Pretty sure this is the one you want to use isIntegratedDeviceBySn() from device

Used like this:

isIntegratedDeviceBySn(this.getSerial())

fullphat commented 2 years ago

Pretty sure this is the one you want to use isIntegratedDeviceBySn() from device

Used like this:

isIntegratedDeviceBySn(this.getSerial())

Thanks - just for my reference, where's this defined?

schliemann commented 2 years ago

Pretty sure this is the one you want to use isIntegratedDeviceBySn() from device Used like this: isIntegratedDeviceBySn(this.getSerial())

Thanks - just for my reference, where's this defined?

On the device object.

fullphat commented 2 years ago

Thanks. I've created a simple Boolean helper for now so I can test whether the station is a T8420 or not; once it's all working properly, this could be removed or improved.

One question: I'm making my changes to the actual .js files so, other than pasting the changes across, how do I get them into the .ts files that appear in my git clone?

Also, do I need to do anything special in the .ts files?

schliemann commented 2 years ago

Don't think so.

schliemann commented 2 years ago

@fullphat

Do you have discord? Or another app. Sometimes easier to chat than github issues.

fullphat commented 2 years ago

I do have discord although I don't use it much/ever. Happy to give it a try though!

schliemann commented 2 years ago

You can PM me at Schliemann#3737

fullphat commented 2 years ago

Done! I'm tcp_port#8306 😊

github-actions[bot] commented 2 years ago

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.