-
Notifications
You must be signed in to change notification settings - Fork 33
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
Adds chip select delay option #209
base: master
Are you sure you want to change the base?
Conversation
Ideally we'd do this kind of thing coprocessor-side with a |
@kevinmehall +1 but in the interest of time, I thought this would be easier (and it does fix a bunch of problems with the Ambient module). |
I created an issue to track the WAIT command in the bridge API: #210 |
@kevinmehall other than that, does this code look good to you? |
this.chipSelect.high(); | ||
this._port.uncork(); | ||
|
||
setTimeout(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally you would only uncork if you wanted a delay, otherwise you're going to get 100us-1ms of delay whether you like it or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your change isn't quite what I meant. In the case where you don't add a delay, you want to leave the entire transaction in one cork
/ uncork
pair so it goes out in a single packet, but when you add a delay, you don't need cork
/uncork
, because you do want the commands separated.
e9356c0
to
ba8621b
Compare
this.chipSelect.low(); | ||
this._port._rx(data_len, callback); | ||
this.chipSelect.high(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the chipSelect.high()
get removed here?
This approach isn't guaranteed to work. The |
if (this.chipSelectDelay > 0) { | ||
this._port.uncork(); | ||
} | ||
}, this.chipSelectDelay); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setTimeout
still has a cost—any other code called after send
but before the end of the execution turn will now execute before any of this code is executed. I think this needs to be reconsidered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above https://github.com/tessel/t2-firmware/pull/209/files#r78751516
@kevinmehall and @rwaldron thanks for the review. Since there are so many concerns and gotchas with this approach, I'll try to go at it from the firmware angle instead. I'm a little unsure as to the best way to complete it because I don't want to hold up the SPI bus. My plan is to implement the |
👍 ping when you need smoke testing :) |
We'll probably replace this PR with #217 but I want to keep this PR around in case I'm not able to get the functionality built properly in firmware. |
Fixes tessel/ambient-attx4#54 and maybe tessel/ir-attx4#32.
The ATTTINY based modules (Ambient and IR) are using a bit-banged version of SPI and need time to configure it prior to receiving data. Otherwise, they cannot reply in time.
This PR adds an optional
chipSelectDelayUs
property which alleviates that problem and makes the API more compatible with the T1 SPI API.