Improved OLED display part with larger hardware support #512
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #512 +/- ##
==========================================
+ Coverage 37.11% 37.17% +0.06%
==========================================
Files 30 30
Lines 5510 5517 +7
Branches 635 634 -1
==========================================
+ Hits 2045 2051 +6
- Misses 3317 3318 +1
Partials 148 148
Continue to review full report at Codecov.
|
|
I already wrote an https://github.com/tikurahul/donkey/blob/donkey-v3-dev/donkeycar/parts/oled.py For reference. |
|
Hello @tikurahul glad, to get feedback from you. I studied your design. The functionality of your OLED part is very nice and showing the IP is a helpful feature. Since the Adafruit libraries use the Linux-Kernel function i2c_smbus_xfer(..) that uses mutex-based protection for concurrent i2c access, your multi-threaded design works without locking up the I2C bus. The idea of my implementation is that it provides an upper run time bound for display updates to meet the soft real-time requirements of Donkey car even for bulky display updates. As a result, complex graphic rendering will not slow down I2c-based actuator controllers. What do you think is a good way to progress? Should I integrate your functionality into your existing OLED driver? Or should the upper bound runtime behaviour added to your OLED-driver implemenation? Thank you for sharing your thoughts, |
|
Thanks for looking at the part. Yes, in general I was trying to minimize the damage caused by a slow part. My version does not try to maintain any real-time guarantees but having used it for a couple of months, that has never been an issue. I can contribute my part to the main |
|
Hello @tikurahul
I fully agree. Please do it like this. After that I will propose an extension to your OLED part with my real-time preserving i2c backend and the graphics functions. By this, we should get powerful and safe OLED support with a straight-forward design. |
|
Thanks guys! Excited to try this.
…On Thu, Dec 19, 2019 at 11:44 AM Oliver Wannenwetsch < ***@***.***> wrote:
Hello @tikurahul <https://github.com/tikurahul>
I can contribute my part to the main donkeycar repo. You can maybe
subsequently make changes you want to, to improve upon the part ?
I fully agree. Please do it like this. After that I will propose an
extension to your OLED part with my real-time preserving i2c backend and
the graphics functions. By this, we should powerful and safe OLED support
with a straight-forward design.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#512?email_source=notifications&email_token=AABKJZ56RKWA2OSF6QTULT3QZPFLFA5CNFSM4J4SCEJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHKWZXY#issuecomment-567635167>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABKJZ6BYM5V7TSIVQJ7OXTQZPFLFANCNFSM4J4SCEJA>
.
|
|
Sorry, will send out the PR today. |
|
Hello @tikurahul thank you for creating the PR. I am looking forward to code on the OLED part on the Christmas break. Best, Oliver |
|
@tikurahul thank you for merging your PR, I will continue to work on it now. |
|
Hello @tikurahul |
|
Ah. I just realized that too. Let me know if you need help when testing things. |
deltaflyer
changed the title
deltaflyer
changed the title
|
Hello @tikurahul I just pushed my changes and updated the PR description. Can you test the code with your display, if you don't mind? My 128x32 display also arrived, so I will test the code also with both displays. |
|
Hey @deltaflyer. Thank you for your PR. You have re-ordered changes, and I can no longer rebase as a result. Can you please squash and reorder your commits so I can rebase them ? (Use |
deltaflyer
force-pushed
the
feature/oled_status_display
branch
from
e29c716
to
5f67f59
Compare
|
Hello @tikurahul I just rebased the commits. Now everything should be fine for review. |
…tus text information
| self.on = False | ||
| self.display.shutdown() | ||
|
|
||
| def get_user_image_as_pil(self): |
There was a problem hiding this comment.
This should be static I would think, given that they are essentially constants.
| def get_canvas(self): | ||
| return self.canvas | ||
|
|
||
| def process_queues(self): |
There was a problem hiding this comment.
Out of curiosity, are there any actionable differences in performance when multiple i2c devices are involved ? Usually actuators are on a different bus, and every bus is modeled as a file handle in the kernel.
Do you have any real benchmarks that measure the difference ? I have been using the OLEDPart and I have not detected any issues thus far.
There was a problem hiding this comment.
Hello @tikurahul,
thank you for your question - they made me think a lot about the design. First, I fully agree to you: the kernel models each i2c access as a own device. Thus, OLED drivers are totally fine in running as a own thread, when other thread use i2c at the same time. Hence, I moved the OLED part into an own thread to speed up the main drive loop with the latest commit.
Following your suggestions, I compared the performance of the existing part and the new OLED part on my Raspberry-Pi 3+ using the 128x32 mode from start up to idle with no loaded h5-model.
Original OLED as baseline
+--------------------+--------+-------+--------+--------+--------+--------+--------+
| part | max | min | avg | 50% | 90% | 99% | 99.9% |
+--------------------+--------+-------+--------+--------+--------+--------+--------+
| OLEDPart | 164.69 | 88.45 | 145.21 | 152.76 | 156.82 | 162.16 | 164.44 |
+--------------------+--------+-------+--------+--------+--------+--------+--------+
Implementation in this PR:
+--------------------+-------+------+-------+------+-------+-------+-------+
| part | max | min | avg | 50% | 90% | 99% | 99.9% |
+--------------------+-------+------+-------+------+-------+-------+-------+
| OLEDPart | 64.53 | 0.09 | 16.17 | 0.10 | 51.88 | 53.69 | 63.02 |
+--------------------+-------+------+-------+------+-------+-------+-------+
On average this the implementation is ~9 times faster. but also the 99% percentile looks promising. This is no suprise, since the low-level stuff is done directly in the part.
Interesting is what happens when the i2c bus gets under heavy load, e.g. when rendering graphics in 128x64 mode in this OLED implementation. If the queue size is set unlimited, the actuators react slow, since the Raspberry-PI hardware cannot dispatch all i2c calls fast enough due to limited system resources. If the process limit size is set to 32 i2c commands per thread call, actuators react smoothly while the display graphic is rendered.
Conclusion:
there is no issue with the existing OLED part. It is doing it's job fast enough and running it in an own thread is totally fine. Thus, I rename the PR. If graphic rendering should be supported on weaker platforms (e.g. Raspi 3+), i2c command queuing and optimized display primitives are needed to have smooth actuator movement. I leave it now open to you and the the other maintainers, if this part is worth the integration. It is quite sophisticated and may not follow the intension of Donkey car that thrives for an simple and straight forward design. Thank your for your time and effort you were spending on this. :-)
… concurrent i2c access
deltaflyer
changed the title
There was a problem hiding this comment.
Thanks for a detailed response.
You tested the original part (I authored) and the new part (with your changes) and both these parts ran as threaded parts right ? This test was performed on a Raspberry Pi 3+ ?
I am confused because in my original PR, the OLEDPart was already a threaded part.
I love performance optimizations like these, so I am not actually opposed to the change. However, I think you might want to refactor this code such that you separate out all the raw i2c communication bits to a different class and that way it does not have to know anything about what is being drawn. It knows how to dispatch the draws and that is the extent of its knowledge.
That way in the future if we wanted to do more with the display, or use the display module independently we can still get all of your performance optimizations.
| self.display.display() | ||
| self.update_requested = False | ||
| self.display.process_queues() | ||
|
|
||
| def update(self): |
There was a problem hiding this comment.
I think you can remove this method ?
There was a problem hiding this comment.
I have to rework this. I will do this by the end of the week.
| ''' | ||
| if x != None: | ||
| return int(math.ceil(x / 400.0)) * 400 | ||
| return int(math.ceil(x / 100.0)) * 100 |
There was a problem hiding this comment.
You could just write this as:
x // 100 * 100
There was a problem hiding this comment.
The good old "//", I rework that. :-)
| @@ -534,7 +534,7 @@ def run(self, mode, recording): | |||
| cfg.SSD1306_ETH_INTERFACE_NAME, | |||
| cfg.SSD1306_WLAN_INTERFACE_NAME | |||
| ) | |||
| V.add(oled_part, inputs=['recording', 'tub/num_records', 'user/mode']) | |||
| V.add(oled_part, inputs=['recording', 'tub/num_records', 'user/mode'], threaded=True) | |||
There was a problem hiding this comment.
I am confused. The original OLEDPart was actually threaded, so did we go from a threaded part -> not threaded -> and not back to threaded again ?
There was a problem hiding this comment.
Yes, indeed. The design went from threaded -> not thread -> thread over the iterations. I benchmarked every iteration to get the best solution on this:
- original OLED part: threaded -> result: high display update rate, decent i2c actuarors, no graphics, noticable CPU usage on Raspberry Pi 3+
- first iteration: reworked not threaded OLED part -> result: poor display update rate, very responsive i2c actuators, graphics support, moderate CPU usage
- second iteration: reworked threaded OLED part -> result: decent update rate, very responsive i2c actuators, graphics support, moderate CPU usage
| @@ -81,7 +81,7 @@ def display_text(self): | |||
| self.canvas.text((x, top), text, font=self.font, fill=255) | |||
| top += 8 | |||
|
|
|||
| def run(self, recording, num_records, user_mode): | |||
| def run_threaded(self, recording, num_records, user_mode): | |||
There was a problem hiding this comment.
Look at the old OLEDPart. The threaded part needs to run in its own thread, and therefore should define a run method. Then run_threaded can update the internal state of the car, and dispatch a call to update. I think you should also make roundUp optional.
There was a problem hiding this comment.
Ah, I see the problem here with my understanding. I will fix this in the next iteration until the end of the week
Dear contributors,
we kindly ask for review of the extension of the OLED part. Following features have been added to the existing OLED-part
It has been tested with the latest dev branch using a Raspberry PI 3 and an Adafruit 16-Channel 12-bit PWM/Servo HAT. Attached are some pictures. After merging of this PR, I can also contribute documentation for the hardware integration. However, integrating the OLED display as another i2c device is complicated
NOTE: the display update rate is slow by purpose to give actuators on i2c priority. if actuators are controlled differently, the update rate can be incremented in the config.
Kudos to Rahul Ravikumar for his initial implementation and the input into the discussion
Thank you for comments and the review,
Oliver