Hi Michael,

Thanks for doing the code review.  Much appreciated.  I really do think this is the best way forward, as I know of no application where the performance will be a limiter.  I think the obd2ecu's use of can3 is probably our current worst-case, and it's not even close to any performance limits. 

I chose to leave in the other buffer checks just in case the chip does something else unexpected under the covers.  Belt and suspenders, if you will, given the odd case I saw with a buffer 1 indication after sending a frame from buffer 0.  There's also some of your original multi-buffer handling further on; fewer code changes now, less risk.  We're close to release, after all.

The Internet's archive show others having troubles, and your research into the use of level (vs edge) interrupts perhaps could be made to work.  But that also seems risky, and without a current need.  I'm not sure I would even mess with the SPI bus frequency (if indeed it is low), given the risk this close to release, though that would be an interesting experiment to see if it would speed things up.  Is the handling of the SPI bus done under hardware control, or does the processor bit-twiddle everything?  If so, a speed-up there could be valuable, if it can be done safely.

To your point about throughput I'm not seeing such a hit.  Wireshark now shows a pretty consistent 780us frame-to-frame for the ECU Name and VIN frames, while in the past this was quite variable, anywhere from 250us to over 600, with one trace at over 9ms which I cannot explain.  But these PIDs are really the exception, and for this application only happen once at startup.  All interactions on the CAN bus from then are request-reply, one frame each, and that timing has not changed as there is nothing to buffer.  I am not aware of any applications where we would need to transmit multiple frames back-to-back on a regular basis. 

Thanks again for your time,

Greg


Michael Balzer wrote:
Greg,

regarding your changes to reduce to TX buffer 1, I think they will do so. I don't think it's necessary to check buffers 2 & 3 busy flags at all, but it shouldn't hurt either.

On the TX limits this introduces: each TX needs…

…total 25 bytes = 200 bits. So at 1 MHz the throughput will be limited to 5,000 frames per second, and the latency will be at least 200 us.

That should still be sufficient for most situations. If you need more speed → can1. Plus maybe the 1 MHz is a typo.

So, while my basic issues still apply, we can use that solution for now.

Regards,
Michael


Am 16.01.2018 um 18:59 schrieb Michael Balzer:
Greg, Mark,

I'm having two basic issues with this:

a) This will limit the TX speed achievable on the mcp interfaces. I generally hate not fully utilizing available hardware capabilities. And this is SPI, filling
a TX buffer is expensive.

b) It again feels like fixing without understanding. I really think we should first understand how the reported race condition of the mcp interrupt signal
affects our driver design, and we should check the effect of level interrupts before resorting to limiting the speed.

The mcp SPI interface is running at 1 MHz, so a TX will … wait a moment, why is it running at 1 MHz? According to the data sheet, it can do 10 MHz?

Mark, is there some scaling done in the spinodma module, or is there a reason (other than typo / copy-paste) for this?



Am 16.01.2018 um 09:47 schrieb Greg D.:

Hi Mark,

Yes, absolutely.  In fact, I just thought of a corner case while nodding
off to sleep (hate it when that happens!).  Fixed, verified, and pushed
to my branch.  Anxiously awaiting review from Michael...

Greg


Mark Webb-Johnson wrote:

I think best for @michael to review. A single tx buffer is certainly easier to manage.

Regards, Mark.


On 16 Jan 2018, at 1:59 PM, Greg D. <gregd2350@gmail.com> wrote:

Mark Webb-Johnson wrote:

In vino veritas.


Indeed!

Fixes look good (can't seem to kill it).  Pushed to my fork on Git Hub,
along with removal of the delays in the obd2ecu.cpp code.

This was done on the older tool chain; wanted to be sure the fix was
real before upgrading and potentially hiding the issue with a slight
change in timing.  Will upgrade the toolchain tomorrow and verify.

Greg

_______________________________________________
OvmsDev mailing list
OvmsDev@lists.teslaclub.hk
http://lists.teslaclub.hk/mailman/listinfo/ovmsdev

_______________________________________________
OvmsDev mailing list
OvmsDev@lists.teslaclub.hk
http://lists.teslaclub.hk/mailman/listinfo/ovmsdev

_______________________________________________
OvmsDev mailing list
OvmsDev@lists.teslaclub.hk
http://lists.teslaclub.hk/mailman/listinfo/ovmsdev


-- 
Michael Balzer * Helkenberger Weg 9 * D-58256 Ennepetal
Fon 02333 / 833 5735 * Handy 0176 / 206 989 26



_______________________________________________
OvmsDev mailing list
OvmsDev@lists.teslaclub.hk
http://lists.teslaclub.hk/mailman/listinfo/ovmsdev