toinsson / pyrealsense

Cross-platform ctypes/Cython wrapper to the librealsense library (v1.x)
http://pyrealsense.readthedocs.io
Apache License 2.0
121 stars 46 forks source link

API extensions #48

Closed papr closed 7 years ago

papr commented 7 years ago

Potentially growing list of API extension:

toinsson commented 7 years ago

nice additions, don't forget to add some docstrings for the documentation (I wrote them for these commits)

2 comments:

papr commented 7 years ago
  1. I guess you meant get_device_options?
  2. StreamMode and DeviceOptionRange are device specific not stream specific. *Stream are currently nice wrapper around streams. Maybe we shoudl add the possibility to initilize them with a StreamMode. But they should not have any controlling logic in them. They are just model classes.

Edit: And yes, I will work on the documentation :)

toinsson commented 7 years ago
  1. I meant get_available_options, but I guess my question comes since I still did not fully understand why you chose to yield results in get_devices, get_device_modes - and thus why get_device_options, get_available_options are not generators themselves. Is that due to the way you intend to use them in your application?

  2. StreamMode, DeviceOptionRange and *Stream are model of objects that do not exists in RS API (by opposition to constants.py and extlib.py), and created here for convenience purpose. Agreed, they should not logic in them, but I still think they belong to the same type of structure. It's a matter of cleaning core.py and refactoring but it is up for discussion.

papr commented 7 years ago
  1. get_devices and get_device_modes enumerate their content using a for loop and calling lrs multiple times. There is a yield after each for-loop-iteration. get_available_options and get_device_options call the lrs api once to fetch all option values efficiently at once. get_device_options should return an iter() (currently a list()) of the ctypes values double array and get_available_options returns a zip generator which is equivalent to yielding the values one by one.

  2. I understand and agree that the namedtuple definitions sould be moved.

toinsson commented 7 years ago
  1. Ok, got it. I am fine with get_device_options returning a list though (only 1 call to lrs). And yielding whenever there is more than 1 call to lrs. I was testing with python2, and zip does not return a generator ... We might need to use six.zip for consistency.

  2. ok, can move these to a new file. Not sure about the name: syntactic.py, sugarclasses.py, lrswrapper.py?

papr commented 7 years ago
  1. In Python 2 zip returns a list. Since it is an iterable, the zip call should be compatible anyway.
  2. Why not move them to utils.py? RealsenseError lives there already.
toinsson commented 7 years ago
  1. yes indeed, zip exists in both but making the behaviour version agnostic is good practise though. Will include six in the dependencies if that's ok with you.

  2. good call.

papr commented 7 years ago

I ususally prioritize short dependency lists. But I agree since six is built-in in Python 3.

toinsson commented 7 years ago

I'll merge everything to master, if any small change you can push to master. After that, I'll make a new distribution for pypi under 2.1.

papr commented 7 years ago

I still have some changes coming: Setting multiple options at once + reset to defaults. I will make a PR shortly.