Bounty: panda J2534 Driver Fix by gregjhogan · Pull Request #84 · commaai/panda

gregjhogan · 2018-02-01T06:36:42Z

This fixes the major issues with getting HDS and i-HDS working.

gregjhogan · 2018-02-01T06:51:22Z

Here is a debug build with instructions on how to set it up
~~j2534-driver.zip~~ (see newer comments)

nanamiwang · 2018-02-02T04:14:41Z

@gregjhogan @diamondman @geohot
A question regarding to the code structure:
I have seen a multi threading Tx/Rx queue mechanism in PandaJ2534Device.h/.cpp, A lot of lines of code are handling queue push/pop and inter-thread communication. This pull request also add another msg process thread.
According to the two docs blew, J2534 DLL client app is restricted to single threaded design:
https://web.archive.org/web/20130805013326/https://tunertools.com/prodimages/DrewTech/Manuals/PassThru_API-1.pdf
page 3

In order to reduce programming complexity the J2534 DLL does
not support multi-threading so the User Application is restricted to a single thread design.

https://archive.org/stream/gov.law.sae.j2534.2002/sae.j2534.2002_djvu.txt
Chapter 6.2

The OEM application is limited to a single thread 
for communication with the tool manufacturer DLL/API. Multiple protocols may 
be connected and communicated on sequentially (serialized) from the single 
application thread. This will prevent the unnecessary complexity of determining 
what message responses belong to which application thread

In my understanding, this rule ensure that client will not send multiple Tx/Rx requests simultaneously, so the Tx/Rx queue mechanism is not necessary, for CAN bus message Tx we can just call WinUSB_WritePipe & WinUSB_ReadPipe(for echo msg recv) directly in PassthruWriteMsgs implementation, and the same for Rx. the whole DLL can be single threaded(maybe periodical msg sending need a dedicate thread). This will greatly reduce the code complexity.
Hoping for your comments.

gregjhogan · 2018-02-02T04:37:39Z

@nanamiwang if there isn't something constantly reading the RX queue off the panda it fills up (on my vehicle in about 2 seconds) and once it is full messages are dropped. Therefore, if you start reading messages only after you transmit a message you are pretty much guaranteed to miss the response because it got dropped.

On top of that, I found that if there wasn't a thread reading messages as fast as possible sometimes a read command would time out because it took to long to get processed through all the code.

I spent a lot of time iterating on making the RX processing more performant until I finally reached a point where HDS worked reliably. I did most of this work while using interrupt based communication (because that is how it was originally configured) but then ultimately ended up switching to bulk mode, so it is possible I went further than I needed to.

I was trying to retain the original architecture as much as possible while dramatically increasing RX processing performance. I will let others decide if a more significant reactor is in order.

gregjhogan · 2018-02-02T04:50:04Z

 	J2534Frame(const panda::PANDA_CAN_MSG& msg_in) {
 		ProtocolID = CAN;
-		ExtraDataIndex = 0;
+		ExtraDataIndex = msg_in.len + 4;


This was changed to match how tactrix operates

gregjhogan · 2018-02-02T04:50:22Z

 				J2534Frame outframe(ISO15765);
 				outframe.Timestamp = frame.Timestamp;
-				outframe.RxStatus = TX_INDICATION | (flags & (ISO15765_ADDR_TYPE | CAN_29BIT_ID));
+				outframe.RxStatus = TX_MSG_TYPE | TX_INDICATION | (flags & (ISO15765_ADDR_TYPE | CAN_29BIT_ID));


This was changed to match how tactrix operates

gregjhogan · 2018-02-02T04:52:30Z

 	this->panda->set_safety_mode(panda::SAFETY_ALLOUTPUT);
 	this->panda->set_can_loopback(FALSE);
-	this->panda->set_alt_setting(1);
+	this->panda->set_alt_setting(0);


Couldn't get enough throughput in interrupt mode (full-speed USB max is 64 bytes per ms)

gregjhogan · 2018-02-02T04:56:17Z

@@ -67,11 +74,28 @@ DWORD PandaJ2534Device::addChannel(std::shared_ptr<J2534Connection>& conn, unsig
 }

 DWORD PandaJ2534Device::can_recv_thread() {


Separated reading USB data from message processing to prevent data reading and message processing from getting behind

geohot · 2018-02-03T04:49:50Z

Nice! We'll test this on TIS on Monday, and if it's good we'll pay out.

Bounty locked.

gregjhogan · 2018-02-03T22:48:33Z

The old zip file only worked if you had visual studio installed, and it was missing a step about adding the DLL directory to your PATH environment variable.

Here is an updated zip with DLLs such that visual studio doesn't need to be installed, and with improved instructions:
~~panda-j2534.zip~~ (see newer comments)

geohot · 2018-02-10T02:39:58Z

Why do we need to add a DLL to PATH? Can we put this in the setup?

gregjhogan · 2018-02-10T02:51:46Z

The registry points to this DLL, so it doesn't need to be in the path:
pandaJ2534_0404_32.dll

and that DLL loads this DLL:
panda.dll

An easy way to accomplish that is to put it in your PATH env var.
The installer (which I haven't figured out how to build yet) puts it in the system32 folder to accomplish this.

gregjhogan · 2018-02-10T02:56:28Z

I was hoping there is an easy way to combine the two DLLs into one DLL, but I haven't found an easy way to do that yet.

gregjhogan · 2018-02-19T17:58:58Z

Figured out how to build the installer, here is a new package:
~~panda J2534 driver install.zip~~ (see newer comments)
(requires panda firmware changes in #87)

Also made the J2534 DLL standalone (for convenience - no need to modify PATH)

gregjhogan · 2018-02-21T03:40:33Z

New installer - this should now work with techstream!

panda J2534 driver install.zip

(requires panda firmware changes in #87)

67d5208 fix signedness issue in toyota safety fe15d3f bump pandacan 11c2b08 add fault invalid 2c26e45 add sleep 27c7637 forgot the counter 3a6d7db don't hang bfa7d2e canloader works b259e2a can flasher is close to working 83f2edf isotp can support in softloader 7ae7c79 typo e85cc47 forgot the selfs 190b4f6 start work on canflasher 5c655c9 add recover support ae3457f usbflash is reliable f7a0ab0 pedal usbflash works 585d0f9 add way to call isotp be82899 despite it being bad code, move isotp 000715b start work on pedal canloader 626e312 pedal has a bootstub now 3662d1e redundant check 81e6b0d fix bug 083cd12 should have bounty to refactor that ish b65d30c bad asserts b2e6c3f isotp untested support for subaddr 30fd66a Merge pull request commaai#93 from vntarasov/volt 06f5109 Merge pull request commaai#94 from gregjhogan/can-printer-hex c7d098c Merge pull request commaai#95 from gregjhogan/setup-script 22fe250 Merge pull request commaai#99 from gregjhogan/bit-transition-example ba16ba3 Merge pull request commaai#100 from gregjhogan/j2534-troubleshooting-instructions ad08ea4 Merge pull request commaai#90 from gregjhogan/can-forwarding f3b6f5d added j2534 troubleshooting instructions 858d150 added script to find bits that transition from 0 to 1 c6acac8 added checking pedal interceptor message length f7226ff added brake safety checks d0c2634 added gas safety checks d378e4a removed bosch safety forwarding restriction on 29 bit addresses 5c7ef9e added bosch safety hooks and forwarding 90c64b6 add note 23de8d4 Merge pull request commaai#97 from commaai/pedal_improvements 0261641 added missing python packages b92b235 fix bytearray encode issue 2434f1c Tweak Volt's brake pedal stickiness e2f73d2 enable has a whole byte to itself d5a9e1e correct checksum f8ed9fa better names 986a14c don't alias pointers 9b8472e add watchdog support 8f0add9 handle faults 1d917f8 split gas set into 2 values, and have a fault state 1b77026 j2534 isn't alpha anymore fbcc872 Merge pull request commaai#92 from commaai/pedal 8a6f44b pedal is sending messages 08f464c python 3 bro is bad bro 9390961 kline checksum algo was broken... 3b7c33b add kline debug support aa622bc init values 631ea9f better refactor eb1fd75 add PEDAL adc sets ccaa310 don't build with usb 8d4d763 debug console works bd09883 comma pedal is building 75a29d5 Merge pull request commaai#84 from gregjhogan/j2534-hds eece37d only the panda has gmlan 9f43abe Merge pull request commaai#89 from vntarasov/volt 5364d43 Merge pull request commaai#88 from vntarasov/smaller-firmware 377a1ec bump version for descriptor fix 4fabdf0 Merge pull request commaai#87 from gregjhogan/usb-multi-packet-control 8580773 fix sending WinUSB Extended Properties Feature Descriptor 6908feb Chevy Volt safety 786a004 Enable optimization to reduce firmware size d70f43b hack to fix thinkpad 95ab1ae fixed flow control message padding bbd04d1 updated installer 62216d0 single standalone DLL for J2534 driver 5c9138d fixed 11 bit address issue f3b0ad2 fix LOOPBACK getting set when DATA_RATE is set b750d36 updated README a9a097f lowered CPU utilization 7c26a70 TIS needs unsupported protocols to return an error 42692b4 TIS doesn't like ChannelID being zero cf126bb SET_CONFIG return error for reserved parameters 2e99dbf fix HDS issues 8203cc8 add is_grey e946a54 add insecure_okay flag 4363b3e check webpage 4f59ded add secure mode note 6b11fb5 add autosecuring to tests b27d185 Merge pull request commaai#86 from commaai/better_pairing 4b53b42 elm wifi isn't an automated test 99f85cb Merge pull request commaai#85 from gregjhogan/usb-wcid 0d38060 auto-install WinUSB device driver c6653ca from python import 38cc0ee add wifi_secure_mode, boots in insecure mode git-subtree-dir: panda git-subtree-split: 67d5208

gregjhogan changed the title ~~fix HDS issues~~ Bounty: panda J2534 Driver Fix Feb 1, 2018

gregjhogan commented Feb 2, 2018

View reviewed changes

gregjhogan force-pushed the j2534-hds branch from 3733826 to 572a380 Compare February 12, 2018 05:35

gregjhogan added 7 commits February 18, 2018 11:19

fix HDS issues

2e99dbf

SET_CONFIG return error for reserved parameters

cf126bb

TIS doesn't like ChannelID being zero

42692b4

TIS needs unsupported protocols to return an error

7c26a70

lowered CPU utilization

a9a097f

updated README

b750d36

fix LOOPBACK getting set when DATA_RATE is set

f3b0ad2

gregjhogan force-pushed the j2534-hds branch from d6983dd to f3b0ad2 Compare February 18, 2018 17:19

gregjhogan added 3 commits February 18, 2018 19:58

fixed 11 bit address issue

5c9138d

single standalone DLL for J2534 driver

62216d0

updated installer

bbd04d1

gregjhogan force-pushed the j2534-hds branch from f31df66 to bbd04d1 Compare February 19, 2018 17:46

fixed flow control message padding

95ab1ae

geohot merged commit 75a29d5 into commaai:master Feb 27, 2018

		@@ -67,11 +74,28 @@ DWORD PandaJ2534Device::addChannel(std::shared_ptr<J2534Connection>& conn, unsig
		}

		DWORD PandaJ2534Device::can_recv_thread() {

Uh oh!

Conversation

gregjhogan commented Feb 1, 2018

Uh oh!

gregjhogan commented Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nanamiwang commented Feb 2, 2018

Uh oh!

gregjhogan commented Feb 2, 2018

Uh oh!

gregjhogan Feb 2, 2018

Choose a reason for hiding this comment

Uh oh!

gregjhogan Feb 2, 2018

Choose a reason for hiding this comment

Uh oh!

gregjhogan Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregjhogan Feb 2, 2018

Choose a reason for hiding this comment

Uh oh!

geohot commented Feb 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gregjhogan commented Feb 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geohot commented Feb 10, 2018

Uh oh!

gregjhogan commented Feb 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gregjhogan commented Feb 10, 2018

Uh oh!

gregjhogan commented Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gregjhogan commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gregjhogan commented Feb 1, 2018 •

edited

Loading

gregjhogan Feb 2, 2018 •

edited

Loading

geohot commented Feb 3, 2018 •

edited

Loading

gregjhogan commented Feb 3, 2018 •

edited

Loading

gregjhogan commented Feb 10, 2018 •

edited

Loading

gregjhogan commented Feb 19, 2018 •

edited

Loading

gregjhogan commented Feb 21, 2018 •

edited

Loading