Driver encapsulation #10
Conversation
No more dobule inheritance! New VideoCapture and VideoDisplay classes to more clsoely match standard OpenCV. New "abstract" base classes for camera and display drivers. All camera and display drivers now encapsulate an interface object for better expandability to other platforms. New utils subpackage with common color modes and memory helper functions. Update rv_init package after refactor. Update DVI exampels to no longer use private members. Add header comments files that were previously missing it. Add full file path to header comments.
…pixel. Fixes a regression with the HM01B0 driver, where the DMA is unable to transfer fast enough to keep up with the PIO when only 1 byte per transfer is performed. It's also just more efficient in general.
On RedBoard RP2350, the D0 pin ends up connecting to the HSTX connector. When an HDMI cable is plugged in, D0 connects to the CEC pin, which has the effect of dramatically increasing the capacitance on the D0 pin, resulting in a terrible eye pattern. Increasing the drive strength helps clean it up a lot. Also probably beneficial to do this to the PCLK pin, and on all boards when using 1-bit mode (highest PCLK frequency, same as MCLK pin) to help maintain good signal quality.
Too high seems to maybe cause oscillations or something. Hard to know for certain, because a scope probe changes the behavior.
| pin_xclk = None, | ||
| ): | ||
| """ | ||
| Initializes the DVP interface with the specified parameters. |
There was a problem hiding this comment.
Update the "Args:" comment to reflect the new changed arguments.
| xclk_freq, | ||
| num_data_pins, | ||
| byte_swap, | ||
| continuous = False, |
There was a problem hiding this comment.
Add a comment saying what these args are similar to what you have for __init__(). Of particular interest is the buffer arg whose type is not immediately obvious when just staring at this function.
Might want to add a short/simple block function comment describing what this function does as well
gigapod
left a comment
gigapod
left a comment
There was a problem hiding this comment.
I marked a few nits, but overall this looks very solid.
Bonus points for the awesome comments and ASCII diagrams.
| continuous = False, | ||
| ): | ||
| self._buffer = buffer | ||
| self._height, self._width, self._bytes_per_pixel = buffer.shape |
There was a problem hiding this comment.
Should buffer be type checked before the assignment statements are performed?
| import array | ||
| from machine import Pin, PWM | ||
| from uctypes import addressof | ||
| from ..utils import memory |
There was a problem hiding this comment.
maybe a better name for this module. memory - especially later in the code - appears to be a build in, native module which is confusing ... Maybe rv_memory, or something ...
Or if this is just internal, so it's accessed using the package name rv, probably not an issue at all
| from .dvp_camera import DVP_Camera | ||
| from time import sleep_us | ||
| import cv2 | ||
| from ..utils import colors |
There was a problem hiding this comment.
Similar as before - maybe a better name for the module, instead of colors. Something that makes it clear its part of this package.
| self._interface = interface | ||
| self._continuous = continuous | ||
| self._num_data_pins = num_data_pins | ||
| super().__init__(i2c, i2c_address, height, width, color_mode, buffer) |
There was a problem hiding this comment.
You could maybe leave a comment here saying "call ../utils/VideoDriver init()" or similar, since this is a pretty nested class, and its immediate parents don't implement an init() of this form. I had to search pretty deep in the inheritance and file structure to see what function was actually getting called here: HM01B0 -> DVP_Camera -> VideoCaptureDriver -> ../utils/VideoDriver. Its okay without it, I think its reasonable for a user to trace the inheritance themselves if they care, but I also don't think it would hurt.
| Returns: | ||
| bool: True if supported, False otherwise | ||
| """ | ||
| raise NotImplementedError("Subclass must implement this method") |
There was a problem hiding this comment.
For extensibility, it might be nice to make this and the resolution_default() follow a form similar to how we handle devices with multiple I2C addresses in the qwiic python drivers, i.e. have a list supportedResolutions or similar and then this resolution_is_supported() function could be something along the lines of:
return (height, width) in supportedResolutions
and the resolution_default() function could be something along the lines of
return supportedResolutions[0] with the understanding that the first value in the derived classes' resolution list should be the default. Then derived classes just have to declare their supportedResolutions list instead of having to implement these functions every time. This really only matters for things that support many resolutions though, which doesn't seem to be the case for most of the existing drivers that only support one.
|
|
||
| def color_mode_is_supported(): | ||
| """ | ||
| Returns the default resolution of the camera. |
| Returns: | ||
| tuple: (height, width) | ||
| """ | ||
| raise NotImplementedError("Subclass must implement this method") |
There was a problem hiding this comment.
See the comment for resolution_is_supported() and resolution_default() we could use that same pattern here as well if desired.
| # Set color mode and bytes per pixel. | ||
| def begin(self, buffer, color_mode): | ||
| """ | ||
| Begins DVI output. |
There was a problem hiding this comment.
Consider mentioning the type and purpose of args here
| return (self._V_ACTIVE_LINES // 2, self._H_ACTIVE_PIXELS // 2) | ||
|
|
||
| def _is_in_sram(self, data_addr): | ||
| def resolution_is_supported(self, height, width): |
There was a problem hiding this comment.
I'm seeing now why you have this as an abstract function/method in parent classes since some of the inherited classes like this one have more complex checks for whether resolution is supported. Because of this, you can keep that raise(not implemented) the way you have now in the parent class if you'd like and disregard my previous comments. Or if you implement the universal check that I suggested in the parent class, you can still override it here for classes that don't follow that pattern and have to perform more complex logic to see what resolutions are supported.
| width = None, | ||
| color_mode = None, | ||
| buffer = None, | ||
| rotation = 1, |
There was a problem hiding this comment.
Consider defining constants for rotation types to avoid magic numbers. I prefer those over just describing it in a comment
Similar to the HM01B0, too high drive strength seems to result in degraded signal integrity, which is not visible with an oscilloscope due to the additional capacitance on the line. The default drive strength was set to the max (4x), which was causing bytes to sometimes be missed with the OV5640. Reducing to 2x seems to be much more stable now, and still sufficiently strong to get a clean eye pattern on the D0 pin while an HDMI cable is connected with the RP2350 RedBoard.
Further testing revealed 2x was still too much. 1x seems much more stable. Also fix erroneous extra bit getting set (not documented in datasheet, so probably shouldn't be changing it).
Resolves #5
The previous camera and display drivers were implemented using double inheritance. One parent was for the device driver (eg. HM01B0 or OV5640), and the other was for the interface driver (eg. SPI or PIO). This meant that to create each combination of m devices and n interfaces, a total of m+n+m*n classes were required, which would get unwieldy quickly.
This changes the implementation so each device driver encapsulates the interface driver (which all implement the same methods), so a total of only m+n classes are required. This also has the benefit of separating interface parameters (eg. pin numbers) from device parameters (eg. resolution) when calling the constructors. Then the user can (theoretically) freely mix and match whatever device and interface drivers they need/want for their setup, which is emphasized in the updated example
rv_initmodule.Other changes:
VideoCaptureclass to more closely match standard OpenCV'sVideoCaptureclass. The constructor takes an object that implements theVideoCaptureDriverabstract base class (which itself, inherits the newVideoDriverabstract base class).VideoDisplayclass to mirror theVideoCaptureclass. This is not a feature of standard OpenCV (cv.imshow()takes a string for the name of a window to display in, instead of an object that represents the window), but IMO it makes sense to keep the display driver structures as similar as possible to the camera drivers, and this is backwards compatible with the existing API difference in MicroPython-OpenCV. The constructor takes an object that implements theVideoDisplayDriverabstract base class (which itself, inherits the newVideoDriverabstract base class).utilssubpackage with modules:colors- Provides standard color mode constants intended for use across all Red Vision drivers, and color-related utility functions.memory- Provides utility functions related to device memory, such as checking whether an object/address is located in internal or external memory (the slower speed of external memory has proven to require different implementations for best performance).pins- Provides utility functions that drivers can leverage to check and modifymachine.Pinbehavior on the fly.rv_initpackage now encourages importing the entire Red Vision package (eg.import red_vision as rv) rather than wildcard imports from submodules (eg.from red_vision.cameras import *) to improve readability and parity with OpenCV (eg.import cv2 as cv).