Skip to content

Aqara-FP2: Add error handling and modify offline logic #2235

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

hyundolee27
Copy link

Check all that apply

Type of Change

  • WWST Certification Request
    • If this is your first time contributing code:
      • I have reviewed the README.md file
      • I have reviewed the CODE_OF_CONDUCT.md file
      • I have signed the CLA
    • I plan on entering a WWST Certification Request or have entered a request through the WWST Certification console at developer.smartthings.com
  • Bug fix
  • New feature
  • Refactor

Checklist

  • I have performed a self-review of my code
  • I have commented my code in hard-to-understand areas
  • I have verified my changes by testing with a device or have communicated a plan for testing
  • I am adding new behavior, such as adding a sub-driver, and have added and run new unit tests to cover the new behavior

Description of Change

  • Added error handling for potential errors that may occur when adding a device.
  • Modified the offline detection logic to align with Aqara's logic.
  • Reset the device exclusion list on every discovery

Summary of Completed Tests

Verified operation with 3 FP2 devices connected at the same time to:

  • SmartThings Hub V2
  • SmartThings Hub V3
  • SmartThings Station
  • SmartMonitor M5's built-in hub

@@ -108,6 +112,7 @@ end

function discovery.do_network_discovery(driver, _, should_continue)
log.info_with({ hub_logs = true }, string.format("discovery start for Aqara FP2"))
processing_devices = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be removed or was it intended to be used?

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your feedback. I have removed this variable. Please review it again.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this was being used and was just a file scoped variable which I think might have caused some confusion. It looks like this was guarding against multiple try add device attempts so I am unsure if this is actually okay to remove or not.

@@ -100,7 +100,7 @@ local function discovery_device(driver)
break
end
end
if (not processing_devices[dni]) and (not is_already_added) then
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like this will cause a lot of extra device added messages from the driver to the cloud if we have already started adding the device to the platform, but the hub hasn't gotten the device record from the platform yet. Why is the processing_devices not needed anymore? It seems like its purpose is to prevent sending multiply try_create_device requests, which seems like something that we want to do.

@hyundolee27
Copy link
Author

@cjswedes @NoahCornell
Thank you for your review.
As you understand, processing_devices is used not to try to register again until device_added is called after try_create_device is called.

And when the connection was not good in the market, there was an issue that it did not appear to be device_added after try_create_device. (Unfortunately, we do not know the exact cause with the remaining logs. )
The existing logic initializes processing_devices only when device_added, so if the device is not added, processing_device remains and cannot be added again.

The first patch was simply a modification to initialize processing_devices for each discovery.
With this modification, if the device registration fails for some reason, I expected that the problem would be solved by canceling the discovery and calling it again.
In this case, once a problem occurs within a certain discovery, it cannot be solved within the same discovery, so it has been modified not to use processing_devices.

However, there is a side effect concern as you mentioned, so I modified processing_devices to be used again.
Instead, I added some complementary logic for processing_devices.

Please review it again including the new commit.

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.

5 participants