simoncahill / isotp-c Goto Github PK

so that 0xF3 st_min will be 300us instead of rounded up to 1ms

https://github.com/Switchleg1/esp32-isotp-ble-bridge/blob/BridgeLEG/main/isotp.c#L9

this person has it working here

I do not see where transmits are able to clear the transmit status.
It appears there may need to be a receive loop that processes the arbitration messages from the receiver during a transmit sequence. Transmit works with a small frame (7 bytes or less) that requires no arbitration.

In addition, I do not see where the uint32_t receive_arbitration_id is ever used.

Reported by Mark,

There is a bug in the documentation concerning the timer values.

I am confused as to whether the return should be in milliseconds or microseconds as both units are referred to in the documentation.

In the code:

        link->send_timer_st = isotp_user_get_us();

In the documentation:

/* required, return system tick, unit is millisecond */
uint32_t isotp_user_get_ms(void) {
    // ...
}

The if condition here

I propose:

/* st_min to microsecond */
static uint8_t isotp_us_to_st_min(uint32_t us) {
    if (us <= 127000) {
        if (us >= 100 && us <= 900) 
        {
            return 0xF0 + (us / 100);
        } 
        else 
        {
            return us / 1000;
        }
    }
    return 0;
}

[Edit:] Same here:

Hi,

I'm in the process of integrating isotp-c into a microcontroller project.

We're using CMake so have included the project as a subdirectory setting the isotpc_STATIC_LIBRARY configuration option.

As noted in the project README, position independent code (PIC) is always forced on:

I was hoping to understand the rationale behind this, as it's typically only required for dynamic libraries.

Additionally flagging the target_compile_options as public means they're propagated to dependencies. Causing build options to sort of "leak" out of the library into the modules which link to it.

In our case our startup code doesn't include any support for PIC, as everything is statically linked. I was surprised to find the system crashing at startup, having linked in isotp-c. Seems its compile definitions had spread and attempted to enable PIC on the module which links to the library causing the system to crash as PIC wasn't properly setup.

For the moment I've just commented out the PIC option but was hoping to submit a PR. Either to make it configurable via cmake, or to only include it when requesting a shared library be built.

I'd propose to make the other compile options private, such that they're only applied to the isotp-c library itself. They're not bad in terms of what they're looking to do. We've got everything but -Wno-unknown-pragmas turned on ourselves already, so aside from PIC didn't really notice them. They probably shouldn't be forced upon the wider project as a whole though.

Would appreciate your thoughts.

Great work on continuing the development of the library btw!

Thanks,

Phil