Lock-up using some ELM compatibles

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Either start the app and go to start live data or run any command from the 
command menu.
2.
3.

What is the expected output? What do you see instead?
The expected output is the result of the command or NODATA.

Regardless of the command, the preamble sent before each command "ate0\n\r" 
will lock-up the adapter as a '>' will never be received but only the partial 
'ate0\n\r' answer. This will end up in the readResult function waiting forever 
to receive a '>' confirmation.

What version of the product are you using? On what operating system?
Latest at April, 2011 on Android 2.2.

Please provide any additional information below.

It seems that some adapters (sorry, I won't name which one as it is a well 
known brand from which I just happened to have one) are really fussy about what 
they receive.
I had more success receiving an answer from the adapter by removing the '\n' 
character appended after each command and leaving just the '\r'. According to 
the ELM specs any command is interpreted when the '\r' character is 
encountered. Thus the '\n' might confuse the adapter and even lock it up. To 
make my particular adapter even more stable I also had to add additional sleep 
periods 100-200ms in various places but I have not yet discovered the minimum 
required periods.

Please note that with some other adapter that I own (cheap elm v1.2 clone), the 
above issues are not present.

Original issue reported on code.google.com by i.anto...@gmail.com on 19 Apr 2011 at 9:42

[GoogleCodeExporter]

A bit of errata and more details:
The received characters before locking-up are "ate0\r\n" and not "ate0\n\r".
The line of code that needs changing is in the sendCmd function just before 
sending a command. The code currently is: cmd += "\r\n"; but removing the '\n' 
improves compatibility with some adapters so maybe it can be coded as an option 
if not enabled by default. 

Original comment by i.anto...@gmail.com on 19 Apr 2011 at 9:52

[GoogleCodeExporter]

I did a bit more debugging and it seems that after removing the '\n' character 
from the sent command and adding a 100ms sleep just before sending the ate0 
command in the while loop from the startDevice() function in the 
ObdConnectThread, my adapter is actually working. Reducing the sleep to just 
50ms was not enough to cure the lockups. I assume that myparticular adapter 
requires some delay between bluetooth being connected and being able to 
properly respond to commands.

Additionally, for some adapters, if you get an error notification just once at 
the begining it might be just because the first command might read just the 
init sequence "ELM v..." that can be sent unsollicited by the adapter on the 
first connection after power up. To cure this, I added in my code, a small loop 
to check for available characters just before sending a command.

Original comment by i.anto...@gmail.com on 20 Apr 2011 at 7:47

[GoogleCodeExporter]

Can you provide a patch for analysis?

Thank you,
PP

Original comment by pjpi...@gmail.com on 15 Sep 2011 at 6:56

• Changed state: Accepted
• Added labels: Priority-High
• Removed labels: Priority-Medium

[GoogleCodeExporter]

Original comment by pjpi...@gmail.com on 22 Sep 2011 at 11:15

• Changed state: Started
• Added labels: Milestone-Release1.3

[GoogleCodeExporter]

Hi PP, 
Nice to see the project is alive :)

I have not worked with the latest version yet, but
I'll try to integrate my changes into the latest code and provide (within 1 
week) at least the changed files. Regarding the patch, what parameters do I 
need to run patch with (on windows) to be suitable for you?.

All the best,
Iosif

Original comment by i.anto...@gmail.com on 22 Sep 2011 at 11:29

[GoogleCodeExporter]

Hi Iosif,

Glad you're still interested in this project :-)

Anyway, I believe there's no more the need for a patch as I think I've fixed 
this. I just haven't been able to commit the changes as I'm performing an heavy 
refactoring to the code (see issue #10) and everything is broken atm. I'll have 
it done by the end of the next week.

Feel free to follow the current dev-1_3 branch and try it yourself.

Also, just for the sake of testing, check this 
[http://dl.dropbox.com/u/143834/obd-reader-1.1.2.1.apk APK] out. The values 
will be shown as hexadecimal but you can see if everything is working and send 
some feedback.

Much appreciated,
PP

Original comment by pjpi...@gmail.com on 22 Sep 2011 at 12:03

[GoogleCodeExporter]

Hi, 
Great news, I have tried the new custom apk and it seems to work OK (I said 
seems because there are still some errors if the protocol is reset to auto and 
the device responds with the "SEARCHING" message. Other than that it works.
I saw you changed the eol to be just '\r' which is essential for my device and 
you also added a sleep for 500ms.

The 500ms might me a bit too much for some devices that do not need it, and 
maybe at some point it will become a configuration option (in my old modified 
code, I had a new settings group named compatibility and there I added this 
configurable delay).

One more note regarding the custom apk, If I try running a single command 
(e.g., Engine RPM), it seems I get no readable answer (I get all kind of 
sending line feeds off and timeout commands OK, but no real result. Maybe that 
is normal at this stage.

I tried compiling the dev branch but as you also mentioned, it did not compile 
properly when I tried it so I could not create a patch and it seems that you 
already have the code in the right place.

All the best,
Iosif.

Original comment by i.anto...@gmail.com on 27 Sep 2011 at 9:28

[GoogleCodeExporter]

Glad to hear it worked. Now, about your comments:

Firstly, the 500ms hard-coded delay is going to be removed completely, since 
I've implemented a "TIME OUT" command "AT ST hh" (hh min=0x01, max=0xFF) that 
will set the maximum time (min=1, max=1200ms) the ELM device wait for the ECU 
to respond to any given command.
The ELM device can even adapt "automagically", but the next release we'll be 
hard-coding a predetermined value.

Secondly, this is the "expected" result for the custom version you just tested: 
when running single commands, the communication with the ELM device will be 
reset. The result itself may vary. Next release (version 1.3) will fix this.

Thirdly, the dev_1-3 branch is outdated until I get a working APK. There has 
been an heavy refactoring (extract commands stuff to pure Java API, rewrite the 
way the Android app establishes and maintains permanent ELM connection, 
unit-testing, etc.). So, you understand why I'm not committing anything..

As I've said, I'm expecting a release candidate version around this weekend.

Original comment by pjpi...@gmail.com on 28 Sep 2011 at 7:21

[GoogleCodeExporter]

Hi PP,

As the 1.3 release seems to be based on quite a major refactoring of code, I 
would expect it would take some time until some bits are ironed out, and 
whenever it is ready please let me know and I'll give it a try. I definitely do 
no ask for you to rush it out, and I do understand that refactoring can take 
some time (usually it is about 2X you think it might take) :).

Regarding the complete removal of the delay, it might still generate problems 
even if the timeout is implemented. In my case, without any sleep I simply do 
not get a complete answer and the application locks up trying to get the line 
terminator.
If this would have been a problem with the AT ST command (like a timeout too 
short), then I would have expected to receive a NO_DATA first before the 
lockup. Again, I reiterate, whenever a new version is ready please let me know 
and I will provide feedback based on my devices.

The following part of the reply it is a bit off topic but related to the 
refactoring you described. I saw you mentioned permanent ELM connections, In my 
opinion this is the way to go if real time visualisation is desired, and I was 
wondering why the app disconnects after every round of pid reads. I assumed it 
was for low power operation (e.g., if you read a set of pids every 10 secs, 
having the connection off in between the reads might save some power or even 
help if you have problems with "noisy" nearby RF (if you are connected less 
time to the obd device, there are less chances to lose the connection). 
Obviously as it is hard to please everyone maybe it will be configurable at 
some point. :)

Also regarding this issue, I remember noticing on my older version that each 
obd command had its own thread when it was run (likely because it had that 
start call and a thread can not be restarted once finished). This behavior is 
bit far from optimal as it is creating/destroying quite a considerable number 
of threads. However, I considered this so far as a minor inconvenient compared 
to a lockup. :)
When you refactor the bt connection, maybe just as a suggestion, the code can 
be changed to have a reading thread and a write thread that are going in loops 
and another one that is submitting commands.  This approach should enable 
saving some overhead.

Once again thank you for the hard work on the project!

Iosif

Original comment by i.anto...@gmail.com on 30 Sep 2011 at 4:36

[GoogleCodeExporter]

Iosif, summing up I pretty much agree with everything you stated, and I can 
assure you there's been a lot of code changes in the past two weeks regarding 
these ideas.

Now, some comments on your thoughts..

Speaking about the major refactoring, so far the API is complete with some 
unit-testing going on. It is a combination of pure Java classes where I 
implement the OBD commands logic (sending command to OutputStream, reading 
response from InputStream and interpreting the result).
At the moment I'm refactoring the Android part (Activities, Services, 
BluetoothSockets, threads, etc.) and I won't commit to the public repo before 
the Android app is 100% functional with the new API.

Next, the delay functionality. Well, what you probably don't know is that the 
ELM interface will give a "NO DATA>" response if your car ECU doesn't respond. 
So, I believe the "AT ST hh" command will suffice! It wasn't implemented in 
previous versions of this project. Let's wait and see when 1.3 comes out. I'm 
counting on your feedback, that's for sure!

Also, I'm going for permanent connection and sequential command execution (send 
and receive), thus not having multi-threaded/asynchronous logic going on. The 
ELM works this way, so trying to do it differently is just a waste of resources 
and added complexity.

And yes, yes, yes!! You're very right on what concerns the threading model 
implemented so far. Once again, my refactoring is removing that part.

Given all these changes, I can only ask your for some patience as something is 
coming out. I just want to be properly tested before I commit something to the 
public tree.

Thank you so much for your feedback and keeping up with me,
Paulo Pires

Original comment by pjpi...@gmail.com on 30 Sep 2011 at 7:34

[GoogleCodeExporter]

This issue was closed by revision 5fec2de19ee1.

Original comment by pjpi...@gmail.com on 28 Oct 2011 at 9:13

• Changed state: Fixed

[GoogleCodeExporter]

1.3-RC1 is available in the Downloads section. Test it and let me know.

Original comment by pjpi...@gmail.com on 29 Oct 2011 at 10:46

[GoogleCodeExporter]

Hi, 
I have tested 1.3-RC1 and it seems to work. And by work I mean I could get some 
readings in the main screen but all the readings were for the same parameter 
and maybe that is expected. Thus, I confirm I did not had the lockup on start 
as it used to be the case previously.

All the best,
Iosif

Original comment by i.anto...@gmail.com on 6 Nov 2011 at 3:29