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.

Sign up for GitHub

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

A work-around for using byte-string as the data #138

Closed
wants to merge 2 commits into from
Closed

A work-around for using byte-string as the data #138

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a7c4f8b
A work-around for using byte-string as the data
EAGrahamJr Aug 2, 2024
8c3ea7e
KISS - just fix the buffer before sending.
EAGrahamJr Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,5 @@ bundles
dist
**/*.egg-info
.vscode
venv
.venv
4 changes: 3 additions & 1 deletion i2cdisplaybus/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,9 @@ def send(self, command: int, data: ReadableBuffer) -> None:
done.
"""
self._begin_transaction()
self._send(DISPLAY_COMMAND, CHIP_SELECT_UNTOUCHED, bytes([command] + data))
# re-wrap in case of byte-string
buffer = list(data) if isinstance(data, bytes) else data
self._send(DISPLAY_COMMAND, CHIP_SELECT_UNTOUCHED, bytes([command] + buffer))
Copy link

Choose a reason for hiding this comment

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

This is going to be super slow. Maybe instead _send could take an optional command argument, and add it to command_bytes before the rest of the data if it's specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are very few instances where a binary-character literal is actually used, since most command/buffer combos are described using real numeric arrays, thus only occurring the overhead of the isinstance check.

Perhaps the more interesting question should be is why did my change on the SSD1306 driver break non-Blinka devices? They should already be able to take numeric-arrays since all the other command-buffer combinations work, what's so special about an empty array vs an empty binary-character literal?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @deshipu, just change _send() to take command directly and skip the extra conversion.

Note that data should not be a list. It should always be a bytes, bytearray or array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, it took a bit of back-tracking to see what y'all were referring to (the usage in _displaycore vs i2cdisplaybus). Changes forthcoming...

Copy link
Member

Choose a reason for hiding this comment

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

Instead of creating a bytearray in send, add an argument to _send(). _send() is allocating a bytearray as well that it can put command into. That way you'll only make one new bytearray, not two.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with @deshipu, just change _send() to take command directly and skip the extra conversion.

Note that data should not be a list. It should always be a bytes, bytearray or array.

Not to be (too overly) pedantic, but I think that's part of the initial issue -- bytes don't work:

>>> from os import urandom
>>> data = urandom(8)
>>> type(data)
<class 'bytes'>
>>> cmd = 0x80
>>> type(cmd)
<class 'int'>
>>> [cmd] + data
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: can only concatenate list (not "bytes") to list

Copy link
Collaborator

@makermelissa makermelissa Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What happens when you send it in 2 calls like in FourWire? This may have been a conversion error when I was updating this class that just happened to test fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just walking through this again, the difference, I believe, is that I2C uses buffered writes, while it appears the FourWire is just jamming byte-at-a-time.

If it's changed to two _send commands (within the same transaction):

# create an array to create a `ReadableBuffer`
self._send(DISPLAY_COMMAND, CHIP_SELECT_UNTOUCHED, [command])
# create buffer and write
self._send(DISPLAY_COMMAND, CHIP_SELECT_UNTOUCHED, data)
# create buffer and write

That appears to require a lot more buffer creation, so even more overhead.

My surmise is that this is because I2C is an addressable format, thus easier to lock and send a bunchastuff at once, while FourWire is chip-select and can tolerate interleaving of bytes.

(Sorry if I'm getting windy, but I recently retired from software development and this the best conversation I've had along those lines in months!)

Copy link
Member

Choose a reason for hiding this comment

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

It also avoids any changes to method signatures and buffer construction elsewhere.

_send() is a private function (designated by the _) used by just this function. It's not a big deal to change its method signature. So, please add command to the signature and then pack it into a buffer inside _send().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'm not comfortable with that change, thanks.

self._end_transaction()

def _send(
Expand Down
Loading