An extra byte is added to the command when requesting Hybrid Battery Pack remaining

[matthew798]

Hello.

I am trying to query a BT obd scanner for the hybrid battery pack state using: batLife = myELM327.hybridBatLife();

The connection is fine, I am using a simulator on an android device, so I can see both ends of the communication.

The issue is that this library send the following command: 015B1 The first 4 bytes are correct, but I am not sure what the last byte is for, and it's causing the obd simulator to ignore the command.

I tried connecting to the simulator using torque, and I can see that the command sent from the app is: 015B This works as I can see the correct value in Torque, and I can see the command and the response in the simulator.

What is the extra '1' at the end of the command? Is this a bug?

[PowerBroker2]

This is interesting - I'll have to take a look at this when I get the chance. That might be a while. If you or anyone else can find the bug first, please let me know here in this issue. I'm open to pull requests, too.

[matthew798]

So this is not a bug. Your library works correctly. It is the "num_responses" byte that indicates the expected amount of responses from the ECU.

From ELM datasheet:

There is a way to speed up the retrieval of information, if you know how many responses will be sent. By telling the ELM327 how many lines of data to receive, it knows when it is finished, so does not have to go through the final timeout, waiting for data that is not coming. Simply add a single hex digit after the OBD request bytes - the value of the digit providing the maximum number of responses to obtain, and the ELM327 does the rest. For example, if you know that there is only one response coming for the engine temperature request that was previously discussed, you can send:

01 05 1

The bug is in the simulator app I am using. Thanks for the quick response and excellent work on this project. Very appreciated!!

[PowerBroker2]

The bug is in the simulator app I am using. Thanks for the quick response and excellent work on this project. Very appreciated!!

Ah, got it, thank you!

[matthew798]

On a related note: Is there a way to query a PID without adding the "num_responses" byte?

I see in the function ELM327::formatQueryArray(uint8_t service, uint16_t pid, uint8_t num_responses) that the num_responses is always added to the query. I think it might be wise to make it optional, especially since the ELM spec considers it optional.

context:

void ELM327::formatQueryArray(uint8_t service, uint16_t pid, uint8_t num_responses)
{
    ...

    query[0] = ((service >> 4) & 0xF) + '0';
    query[1] = (service & 0xF) + '0';

        ...

    // determine PID length (standard queries have 16-bit PIDs,
    // but some custom queries have PIDs with 32-bit values)
    if (pid & 0xFF00)
    {
        query[2] = ((pid >> 12) & 0xF) + '0';
        query[3] = ((pid >> 8) & 0xF) + '0';
        query[4] = ((pid >> 4) & 0xF) + '0';
        query[5] = (pid & 0xF) + '0';
        query[6] = num_responses + '0'; <-- This should maybe be optional
        query[7] = '\0';

        upper(query, 6);
    }
        ...

I'm thinking adding a default value for num_responses I.E.: void ELM327::formatQueryArray(uint8_t service, uint16_t pid, uint8_t num_responses = 0)

and then:

if (pid & 0xFF00)
    {
        query[2] = ((pid >> 12) & 0xF) + '0';
        query[3] = ((pid >> 8) & 0xF) + '0';
        query[4] = ((pid >> 4) & 0xF) + '0';
        query[5] = (pid & 0xF) + '0';
                if(num_responses){
            query[6] = num_responses + '0';
            query[7] = '\0';
                }else{
                    query[6]='\0';
                }

        upper(query, 6);
    }

I'd be happy, no, thrilled to submit a PR if you think this is a good idea.

[patfelst]

correct, I added the "1" at the end of the command when I did the non-blocking code update, it significantly speeds up response times.

It was actually my intention to make it optional, i.e. if you call the command with a num_responses == zero, it should not be appended.

[PowerBroker2]

I'm always open to PRs

[patfelst]

PS @matthew798 I suggest making the default value "1" as this will be beneficial for the majority of library users. People who care - like you - can dig a bit deeper and call the command with it set to zero.

In either case, this should be added as an explanation on the README.me under "Concept of Execution"

Thoughts?

[matthew798]

@patfelst Just off the cuff, I think that might complicate things for people who don't read the docs. I'm thinking someone might leave the parameter default (1) and unsuspectingly be dropping any responses after the first. This is just an assumption though as I'm not very familiar with the ELM327 protocol. Having a default 0 will only cause some (all?) queries to take longer, whereas 1 might cause important data to be skipped.

[patfelst]

I just had another look at this. When we did the non-blocking library change, we actually added the num_responses parameter to the whole chain of functions starting from a user calling a PID request, all the way down to formatQueryArray, it's just that, as you pointed out @matthew798, I forgot to include a check for num_reponses == 0 and not add it to the end of the query string.

For example in the PID query for RPM

float ELM327::rpm()
{
    return processPID(SERVICE_01, ENGINE_RPM, 1, 2, 1.0 / 4.0);
}

that 3rd parameter "1" is num_responses which gets passed all the way down to formatQueryArray. So to make this an optional parameter, you'd need to modify (I think) all PID functions (about 100 of them!) to make num_response optional.

Also the 4th parameter "2" is the expecged number of bytes in the response numExpectedBytes, and I believe unless this is > 4 bytes (might be 5 bytes), then you'll always get only one response. All of the supported PIDs in @PowerBroker2's library have a max number of 4 response bytes, so for the supported PIDs num_responses == 1 will always work and give the benefit of improved frame rates to the "casual user" of the library.

You are correct though, if an advanced user constructs their own PID query for a PID not in this library, and that PID returns a multi-line response (i.e. more than 4 bytes long), with num_responses == 1 they will only recieve the first response line, and miss the subsequent line(s). But I still think any developer going to that length is going to spend the time to study the library source and realise they need to adjust num_responses == x where x > 1. They are also going to have to write their own function to decode that multi-line response, like I did in the VIN query funtion which returns a 3 line response.

[matthew798]

@patfelst What if we went one slot lower on the stack?

bool ELM327::queryPID(const uint8_t& service, const uint16_t& pid, const uint8_t& num_responses)

This function is called by processPID, and we could easily provide a default for num_responses. Anyone using processPID will of course be able to keep doing so, but new code should use queryPID in stead of processPID.

[patfelst]

I think that would work. So to clarify, with your proposed change, are you saying an advanced user could call queryPID(service, pid) i.e. leaving off the num_responses and by default it would be zero and therefore no num_response byte would be added to the end of the query string? If they wanted a 1 added they would call queryPID(service, pid, 1).

And for all "normal" users of the library, they'd continue calling the PID functions as they currently exist, and would get the 1 added? If so yes this would be a good solution.

[matthew798]

Yes that's what I'm thinking. I could try to find find time to work on it but time is scarce when you have 2 young children 😊