rsjudka / intelligent-auto

41 stars 15 forks source link

Bluetooth OBDII Implementation #18

Open icecube45 opened 4 years ago

icecube45 commented 4 years ago

Implemented to the same level as the usb OBDII (no setting to change adapter yet) - also comments out usb adapter initialization, but added the skeleton of adapter type. Tested/working on a cheap elm BT device in my Infiniti.

rsjudka commented 4 years ago

Wow I didnt know Qt had a library for this! It also looks like it has a library for Serial too.

My original intentions for this OBD stuff were to have it be vanilla C++ so that way it could be reused for other OBD-related purposes. But maybe to keep things consistent we just use Qt everywhere?

So I guess my question for you would be do we keep this vanilla c++ and "convert" your code to just use the bluetooth/rfcomm library, or do we go all out with Qt and "convert" my code to use the Qt Serial library?

icecube45 commented 4 years ago

Is it fair to say neither option is best? I'm in the camp of - the project already uses Qt, use Qt where applicable. When I was looking into methods of using bluetooth for this, Qt seemed the most user friendly, as every rfcomm implementation I could find needed OS side configuration.

But this implementation also isn't "true" Qt - instead of using Qt's provided read/write there's a bit of a hacky solution of grabbing the socket descriptor - this was done because Qt's read/writes are done buffered and only processed when the main event loop is returned to - this was incompatible with the way the OBDII class was written.

So I'm not sure what the correct way forward is, as it's likely using a Qt serial class will necessitate the same "hacky" solution.

On a different note: Bluetooth OBDII right now is.. slow - not sure if that's a limitation of the adapter itself, or an issue with the implementation - will keep researching/testing.

rsjudka commented 4 years ago

Qt seemed the most user friendly, as every rfcomm implementation I could find needed OS side configuration

This is a good point, especially since a goal is to abstract as much configuration from the user as possible. This would also help with hopefully getting it to build for WIndows (seems like Qt has limited support for it atm tho).

this was done because Qt's read/writes are done buffered and only processed when the main event loop is returned to likely using a Qt serial class will necessitate the same "hacky" solution.

I dont really see why the hacky solution is needed tho... Were you running into issues just using the read/write functions?

I honestly wouldnt mind if the OBD class was rewritten (i understand if you wouldnt feel like doing it tho :p). Especially digging into the QIODevice class, I can see alot of benefits to using Qt for all the OBD communication. It would also help alot with keeping things clean because it would be the same commands whether you are using a USB or bluetooth device.

icecube45 commented 4 years ago

this was done because Qt's read/writes are done buffered and only processed when the main event loop is returned to likely using a Qt serial class will necessitate the same "hacky" solution.

I dont really see why the hacky solution is needed tho... Were you running into issues just using the read/write functions?

Because using the default write, for instance, will just queue commands to be sent, until Qt's event loop is ran to process them.

So for the example of initialize() - the function sends the initialization commands (ATD, ATZ, ..., 0100), and expects a response from each command before proceeding. With the asynchronous read/write Qt provided, those commands would simply get queued and sent all at once, and we wouldn't receive data from read() until all of them had been sent.

Essentially - there's no Qt way to flush the read/write buffer - by grabbing the socket descriptor, we can do read/writes in real time within the functions.

If we were to rewrite the OBD class from the ground up, this likely could be gone away with, but since I was trying to preserve compatibility with the USB implementation, this was the best solution I found.

rsjudka commented 4 years ago

Ah I see I had to dig a bit deeper into the documentation to see what you were talking about.

I feel like if we were to add this feature, it would be better to spend a little extra time rewriting it to make it more polymorphic and utilize the additional features Qt provides to optimize some of the reads and writes.

stefan-sherwood commented 4 years ago

Bluetooth OBDII right now is.. slow - not sure if that's a limitation of the adapter itself, or an issue with the implementation - will keep researching/testing.

OBDII speed varies widely with both the car and the Bluetooth adapter. Ideally it should refresh 8-ish times per second. I've seen >1 second refreshes, even with USB.

stefan-sherwood commented 4 years ago

This should be merged into develop, no? Is there anything left to do here?

Edit: other than finding a way to get 25 hours in a day :O

icecube45 commented 4 years ago

This should be merged into develop, no? Is there anything left to do here?

Edit: other than finding a way to get 25 hours in a day :O

Afaik, PR has been abandoned in favor of complete OBD/CAN overhaul. I've just been waiting on parts to interface with my car before I start tackling that.