iwanders / OBD9141

A class to read an ISO 9141-2 port found in OBD-II ports.
MIT License
227 stars 70 forks source link

I just discovered that I "reinvented" the wheel #15

Open aster94 opened 5 years ago

aster94 commented 5 years ago

Hi Ivor,

I had a GSX-R 600 2011, which uses the K-line. Almost a year ago i started writing some code to comunicate with the ECU, probably i didn't found your library because you implemented the KWP recently. Anyway I see a few differences between your init sequence and mine (especially with the init message, the sequence is almost the same)

Could i ask you were did you found the resources to write them?

I am writing also because I am sure i will take some idea from your code, i really like your management of the delay 😃

here it is my library if you would like to have a look: https://github.com/aster94/Keyword-Protocol-2000

iwanders commented 5 years ago

Hi!

Almost a year ago i started writing some code to comunicate with the ECU, probably i didn't found your library because you implemented the KWP recently.

Yeah, it was merged last summer, so after you started work on your library. I also think the name of this library may not make it immediately clear that it also support KWP :) Maybe it is feature creep..? :thinking:

Anyway I see a few differences between your init sequence and mine (especially with the init message, the sequence is almost the same)

Hmm, you seem to send:

format_physical, ECU_addr, OUR_addr, start_com
0x80, 0x12, 0xF1, 0x81

Whereas my init sequence sends:

{0xC1, 0x33, 0xF1, 0x81};

The 0xC1 is close to what you called the format_functional, I think the 0x33 can be anything really, as long as it is consistent. Yours has a lot more diagnostic output though!... Mine is mostly; it either works, or you have to enabled the debug flag and get into the nitty gritty details.

Could i ask you were did you found the resources to write them?

The initial work I did on the init function was a blind implementation, (I don't remember what I based it on, probably some quick googling), in 6622e8e8a2d9d4af719367a16bc803095c3cdc21 and #5, someone requested it, but I never heard back. Finally in #11, a collaboration paid off and with some back and forth we managed to get it to work. Take a read through #11 , there's some information in there that was used to get it to work. I've never owned a vehicle that used KWP, so that thread is all the information I have used. The original development of this library was done by looking at the 9141-2 communication from cheap bluetooth OBD dongle, the logic analyzer traces are in this repo as well.

I am writing also because I am sure i will take some idea from your code, i really like your management of the delay smiley

Always happy to hear something like that. Yeah, I also really like the timing in my library, it's almost always as fast as it can be, without having to tune any values. So definitely take the concept of using readBytes and apply that to your library as well :+1:

zakirsheik commented 5 years ago

@aster94 Does your library work with KWP Fast Protocol ?

aster94 commented 5 years ago

The 0xC1 is close to what you called the format_functional, I think the 0x33 can be anything really, as long as it is consistent.

True! tecnically (in KWP) it should be between hex 10 and 17 the other values are reserved but I think you are right it "can be anything"

Yours has a lot more diagnostic output though!

They are there because at the beginning I had a lot of troubble to make it working 😂

So definitely take the concept of using readBytes and apply that to your library as well 👍

thanks!

If you want i have a few documentations about the KWP, I think that i have an old version of this document that cost around 140€! I found it on google but i didn't add in my repo because i wasn't sure if i was allowed to pubblic it 😅

Maybe it is feature creep

Really i was also thinking to join the efforts and make a unique library that under the hood uses some basic functions like readBytes() and a general abstracted layer like a init() function that basing on the constructor uses the KWP or the 9141, or the CAN-bus (i think that recently more and more constructors are using this protocol). It would be quite cool 😃

I already abstracted my library, for example with a general function requestSensorsData() sends different pids basing on the manufacter

BUT my lib is only tested on suzuki motorbikes! I never had the possibility to test on other even if the basic methods would be the same and only the PIDs should be changed

aster94 commented 5 years ago

hi @zakirsheik I was writing while you posted your comment, i implemented only the fast init, but be aware that, as i wrote in the last comment, you would need to discover some pids that are specific for your vehicle (what is it by the way?) this is the real problem of KWP every manufacter made what they wanted! have a look here so for example in my suzuki motorbike i can get the rpm asking to the ECU 0x71 while on a honda motorbike i would need to ask 0x90 (not real values)

But even before starting all this if i were you i would start by checking with the logic analyzer how your chip manages the baud at 10400, for example using the esp32 with the arduino framework i found that the problem was outside my library https://github.com/espressif/arduino-esp32/issues/2004

iwanders commented 5 years ago

I think that i have an old version of this document that cost around 140€! I found it on google but i didn't add in my repo because i wasn't sure if i was allowed to public it.

I'm pretty sure their legal team will go after you if you make it public or share it. I thought ISO norms were bound to a person or organisation on purchase and non transferable. So I'd stay clear or distributing it.

Really i was also thinking to join the efforts and make a unique library that under the hood uses some basic functions like readBytes() and a general abstracted layer like a init() function that basing on the constructor uses the KWP or the 9141, or the CAN-bus (i think that recently more and more constructors are using this protocol). It would be quite cool

I expect there's already libraries out there to do OBD with a CAN bus. I'm not too interested in doing more development on reading from OBD ports myself. This library in its current form is well tested, mature and simple to understand. It's MIT license, so feel free to fork it, or reuse parts, as long as you adhere to the license everything is fair game.

I already abstracted my library, for example with a general function requestSensorsData() sends different pids basing on the manufacter

Ah, you used #ifdef, that's good, that way the other brands don't increase your flash size. I see you do the conversion of values there as well, this is another reason why I stopped at the protocol layer, because doing the conversion requires floating point support to be provided during compilation. Since the Teensy 3.2 or Arduino doesn't have an FPU this is done in software and ends up consuming quite a bit of flash. Projects may not need the exact conversion, in my screen example I just use the integer conversion, it does require some more parenthesis and possibly loses some precision, but it steers clear of the floats, which means I can store those bitmaps in the microcontrollers' flash :)

I do believe simple and small libraries are preferable, because changing them is doable even if you haven't build them yourself. Same with one or multiple protocols, I'll pick the single protocol one, simply because the additional protocols will only take up more memory or add layers of abstraction which are unnecessary in my use case. My projects target just one vehicle (the one that's mine). Now, I'm not saying one is better than the other, it's just different design goals and personal preferences.

If you do end up building something generic, the folks at https://www.macchina.cc/ may be interested in hearing about it, their hardware supports multiple physical buses. I think they point to my library for 9141-2.

for example using the esp32 with the arduino framework i found that the problem was outside my library

That's pretty bad, good to know that custom baud rates may be problematic! That's not something you expect to fail.

aster94 commented 5 years ago

I do believe simple and small libraries are preferable, because changing them is doable even if you haven't build them yourself. Same with one or multiple protocols, I'll pick the single protocol one, simply because the additional protocols will only take up more memory or add layers of abstraction which are unnecessary

yeah that is true, yesterday i had this idea but i also understand pretty well all the limitation and cons that it has. Mainly i like the idea because i am a fan of standardized API 😅

Ah, you used #ifdef, that's good, that way the other brands don't increase your flash size

exactly, but to make the library more user friendly i may switch to normal if() ... else if() all depends on how much flash size would it take, my goal is to make this lib also usable on arduino UNO

That's pretty bad, good to know that custom baud rates may be problematic! That's not something you expect to fail.

I lost 4 days for this reason, I also already drawed a pcb with esp32 (luckly i didn't make it) then i had to completely change architecture 😢 as you said i didn't expect it to fail. The sad part was that none from the esp32 developers looked into the problem because "no one needs custom baud rates"

Keithjsy commented 5 years ago

Hi Thanks for creating your library, it seems to be just what I'm looking for. However, I'd be really grateful for a bit of advice about using it. I'm trying to communicate with a VW marine engine which I know uses KWP. My issue is not knowing whether it is slow or fast init though. Does that matter with your code? I have tried both reader and KWPreader without success. I'm fairly confident my transceiver chip (L9637) is working as I can see a level shift during the init attempts with my oscilloscope. I've tried setting the debug option but so far I'm not getting any more info on the serial monitor but that might be an IDE issue that I need to sort as it doesn't seem to be using the edited OBD9141.h that resides in my libraries folder. Your advice would be appreciated

iwanders commented 5 years ago

My issue is not knowing whether it is slow or fast init though. Does that matter with your code?

Yes it does, I think... I only implemented KWP fast init, I don't know what the slow init would look like.

I've tried setting the debug option but so far I'm not getting any more info on the serial monitor but that might be an IDE issue that I need to sort as it doesn't seem to be using the edited OBD9141.h that resides in my libraries folder.

The Arduino IDE isn't the greatest when it comes to this, it keeps a copy of the library in a temp folder. I don't know where this resides on windows, on linux it's somewhere in /tmp/. Otherwise, don't use it as a library, just copy the OBD9141.{h,cpp} file to the sketch directory, that should always pull in the local files.

Having debug enabled should give different output. Not sure if the marine engine would have a different handshake though... I don't know anything about those :)

Keithjsy commented 5 years ago

Thanks for your advice Ivor. Presumably (if I can learn the finer details of coding) I could do a bit of cutting and pasting and combine the KWP fast init handshake into the 5 baud slow init sketch or vice versa. Incidentally, I can communicate with the marine ECU with my VW based car diagnostic reader so I'm pretty sure the handshake is nothing special.

rocketjosh commented 5 years ago

Hey @iwanders - That's funny, I was just working on updating our board manager files to include your library automatically! @aster94 if you are working on different protocols and could use some hardware, just let me know.

aster94 commented 5 years ago

Nope, for now I will just focus only on KWP2000, after suzuki now i am working on the kawasaki side! my library is compatible with all the architectures so if your boards are compatible with arduino they would work without problems 🙂