-
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
[WIP] Adds delay command to firmware #217
base: master
Are you sure you want to change the base?
Conversation
@kevinmehall I could use your input here. |
this.chipSelect.low(); | ||
this._port.delay(this.chipSelectDelayUs); | ||
this._port.cork(); |
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.
no need to cork and uncork around the delay -- submit it all as one batch
} | ||
|
||
// Enable delays on this TCC channel | ||
tcc_delay_enable(p->port->delay_id); |
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.
are the TC and TCC compatible enough to use the same function and register struct for both?
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.
That's a good question. I'm not sure why I made that assumption. I'll look into it.
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.
@kevinmehall is there any reason we can't re-use the TC_BOOT TC for port B? Then they could both use TC
s instead of having to implement switching logic for TCs and TCCs.
I'll look more closely tonight. The reuse of the TCC and split use of TC + TCC are kind of ugly. We also now have 5 timers used for one-shot events that could be multiplexed onto a single timer. Here's the timer queue code from T1 if you want to go that route: https://github.com/tessel/t1-runtime/blob/dd434be5499ed04709d78a0eba497f908e2cc235/src/tm_timer.c |
// Pack the buffer with delay bytes | ||
microseconds.writeUInt32BE(delayUs); | ||
// Send off the delay command | ||
this.sock.write(Buffer.concat([new Buffer([CMD.WAIT]), microseconds])); |
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.
Instead of making two buffers, create a single 5 byte buffer and write the bytes into it:
var frame = new Buffer(5);
frame.writeUInt8(0xFF);
frame.writeUInt32BE(delayUs, 1);
this.sock.write(frame);
Here's the difference when running on a Tessel 2 (1 execution each time):
...
INFO Running measure-buffer-allocation.js...
title result
--------------------------------- ---------
Two Buffers, one write and concat 0s 20.7ms
One Buffer, two writes 0s 2.86ms
...
INFO Running measure-buffer-allocation.js...
title result
--------------------------------- ----------
Two Buffers, one write and concat 0s 20.66ms
One Buffer, two writes 0s 2.8ms
...
INFO Running measure-buffer-allocation.js...
title result
--------------------------------- ----------
Two Buffers, one write and concat 0s 20.82ms
One Buffer, two writes 0s 3.04ms
(https://gist.github.com/rwaldron/23a0ea37641138f7bf0d04cc9bda6d34)
More importantly, this code must not be reachable if the delay is zero.
this.chipSelect.low(); | ||
this._port.delay(this.chipSelectDelayUs); |
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.
Wrap both of these in a condition, because if the delay is zero, we definitely do not want to waste the 3ms sending a message that effectively means "don't do anything"
this.chipSelect.low(); | ||
this._port.delay(this.chipSelectDelayUs); | ||
this._port.cork(); |
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.
Take a look at this gist: https://gist.github.com/rwaldron/4609336bb152388260c753d8e176fa8a
The average execution of _port.cork(), over 10 measurements is about 2.74ms, which means that an SPI object created with a chipSelectDelayUs that has any value less than 2740μs is always unnecessary (and indeed detrimental), because the call to cork has already taken that amount of time. I add that it's detrimental, because it's adding 2.5-3ms to the execution—even when the delay is zero.
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.
This might also be why your logic analyzer results are coming up inconsistent
Is there any progress planned on this? I came here via tessel/ambient-attx4#54 and I am seeking for a fix. This PR is mentioned as the basis for stabilising the ambient module... |
@xenji Unfortunately, I don't plan to work on this anytime soon. I think the best way to make the ambient module stable for now is to read data from it at the lowest frequency possible. |
In order to have a delay function, we need two timers so that both ports can run delays at the same time. We only had one unused timer left so I had to "recycle" the timer we use for the boot LED animation sequence (TCC1). Also, I renamed the unimplemented
CMD_GPIO_WAIT
to the more genericCMD_WAIT
.This still does not work. I was testing with the following function and a logic analyzer to confirm delay time:
I immediately get the following error:
130 equals 0x82 which correlates to
PortReply.REPLY_HIGH
.Also, running the same function repeatedly results in an inconsistent delay that usually is not related to the delay time I requested.
It would be useful to get more eyes on this to try to figure out what I'm doing incorrectly.