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

Memoize attribute listeners to not call them on equal values. #446

Merged
merged 3 commits into from
Nov 11, 2015

Conversation

tcr3dr
Copy link
Contributor

@tcr3dr tcr3dr commented Nov 11, 2015

See #60. Memoization seemed like the "cheapest" approach: not up to the end user to maintain state and observe changes, instead, implemented at the level of attribute listening.

@hamishwillee hamishwillee changed the title Memoize attribute listeners to not call them on equal values. Memorize attribute listeners to not call them on equal values. Nov 11, 2015
@hamishwillee
Copy link
Contributor

Fantastic.. I thought we'd decided to just live with this. - pleased to be mistaken. I've added the docs and example updates to support this change.

I guess I should point out that this is good but not silver bullet for users. Some attributes like gps location will drift a bit and show changed values even though the real position has not changed. Still pretty cool.

ISSUE: Only issue is that the vehicle_state.py example has the following crash while waiting for home position first time you connect. This happens if you run the example against dronekit-sitl copter first time (of anything that is connecting to the SITL). After if fails you can connect again and you don't see this problem...

 Mode: STABILIZE
 Armed: False
 Waiting for home location ...
Traceback (most recent call last):
  File "E:\deleteme\dronekit-python\examples\vehicle_state\vehicle_state.py", line 61, in <module>
    cmds.wait_ready()
  File "build\bdist.win-amd64\egg\dronekit\lib\__init__.py", line 1571, in wait_ready
  File "build\bdist.win-amd64\egg\dronekit\lib\__init__.py", line 1368, in wait_ready
dronekit.lib.APIException

However you see it again when the code gets a bit further - same problem with downloading waypoints ...

Set new home location
 New Home Location (from attribute - altitude should be 222): LocationGlobal:lat=-35.363261,lon=149.1652299,alt=222,is_relative=False
Traceback (most recent call last):
  File "E:\deleteme\dronekit-python\examples\vehicle_state\vehicle_state.py", line 80, in <module>
    cmds.wait_ready()
  File "build\bdist.win-amd64\egg\dronekit\lib\__init__.py", line 1571, in wait_ready
  File "build\bdist.win-amd64\egg\dronekit\lib\__init__.py", line 1368, in wait_ready
dronekit.lib.APIException

@tcr3dr
Copy link
Contributor Author

tcr3dr commented Nov 11, 2015

@hamishwillee So I updated this PR again (it again passes tests). This is actually much trickier than I expected, partly because some objects don't change their equality (.commands for example) and others are more like "sensor" readings than arbitrary updates.

So I added a cached=True option for notify_attribute_listener as an opt-in. I think we can identify which are the obvious culprits (here, all the attributes set as part of HEARTBEAT) and maybe a few more, and ship with that. We should hedge toward too many notifications rather than accidentally too few.

@tcr3dr tcr3dr changed the title Memorize attribute listeners to not call them on equal values. Memoize attribute listeners to not call them on equal values. Nov 11, 2015
@tcr3dr
Copy link
Contributor Author

tcr3dr commented Nov 11, 2015

@hamishwillee Also "memoize" is the correct term, not memorize 👼

@hamishwillee
Copy link
Contributor

@tcr3dr Learn something new every day - "memoize" indeed!

That sounds like a pragmatic solution to an ugly problem. I'll update the documents now. I guess it was probably a good idea to dump my last set of changes.

@tcr3dr
Copy link
Contributor Author

tcr3dr commented Nov 11, 2015

@hamishwillee Did you upload a change I overwrote again? Gah.

@hamishwillee
Copy link
Contributor

@tcr3dr Nope, not overwritten anything yet - though you did kill my last changes. I am about to update the docs now.

@hamishwillee
Copy link
Contributor

@tcr3dr Docs updated now. They need a scan from you - my brain starting to ooze :-(

@tcr3dr
Copy link
Contributor Author

tcr3dr commented Nov 11, 2015

@hamishwillee Reviewed the docs—they look stellar, so good work!

tcr3dr added a commit that referenced this pull request Nov 11, 2015
Memoize attribute listeners to not call them on equal values.
@tcr3dr tcr3dr merged commit be9f4c2 into master Nov 11, 2015
@tcr3dr tcr3dr deleted the tcr-attrup branch November 11, 2015 19:54
@hamishwillee
Copy link
Contributor

@tcr3dr Gad you like them. I needed the check as I noticed I'd been sloppy with some example cleanup ... happens more as you get tired.

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.

2 participants