search

min-protocol / min

The MIN protocol specification and reference implementation
258 stars 88 forks source link

VS 2015 min_debug_print(args...) + Suggestions ? #14

Open erakis opened 6 years ago

erakis commented 6 years ago

Hi,

On Visual Studio 2015, this does not compile

#ifdef MIN_DEBUG_PRINTING
// Debug print
void min_debug_print(const char *msg, ...);
#else
#define min_debug_print(args...)         <-------- THIS
#endif

Give the error

Error   C2010   '.': unexpected in macro formal parameter list

To fix it replace by.

#define min_debug_print(...)

Suggestion 1

Sometime we only want to reset locale and not remote. Can you add a boolean to the function min_transport_reset(bool sendToOtherSide) ?

Other alternative would be to expose function transport_fifo_reset to min.h.

Suggestion 2

For some reason I would like to handle heartbeat on application side. Can you add a define to deactivate idle (periodic) ACK ?

#ifdef TRANSPORT_ACK_RETRANSMIT_ENABLED
    // Periodically transmit the ACK with the rn value, unless the line has gone idle
    if(now - self->transport_fifo.last_sent_ack_time_ms > TRANSPORT_ACK_RETRANSMIT_TIMEOUT_MS) {
        if(remote_active) {
            send_ack(self);
        }
    }
#endif
kentindell commented 6 years ago

Those look like good suggestions.

erakis commented 6 years ago

I would tell you that the most important is the compilation bug, can you fix it ? Else I would need to fork your project...

About the suggestion, I can live without but I think they will bring more flexibility to the API.

How much time do you plan to do the fix and API ? Or do you want me to make a PR for

For suggestion 2 I'm now sure if there something else to do except the fix I proposed.

kentindell commented 6 years ago

Done all three fixes: https://github.com/min-protocol/min/commit/4626d73aee585687a3d40a8ff0f70690e05fc506

The compilation option is to suspend periodic ACK retransmission rather than include it so that the default is to include it as at present.

erakis commented 6 years ago

Wow, thank your so much @kentindell.

Last thing

Suggestion 3

What do you think to let us use a lookup table to compute CRC32 instead of expensive nested for instruction (bytes + bits) ? If size is an issue on embedded side, we can use a macro to indicate that we don't want to use table and fallback to your old method ? Something like this

#ifdef MIN_TRANSPORT_PROTOCOL

  static void crc32_init_context(struct crc32_context *context)
  {
      context->crc = 0xffffffffU;
  }

  #ifdef MIN_TRANSPORT_USE_CRC_TABLE

    static uint32_t CRC32TableSize = 256;
    static uint32_t CRC32Table[CRCTableSize] =
    {
        0x00000000, 0x77073096, 0xee0e612c, 0x990951ba,
        0x076dc419, 0x706af48f, 0xe963a535, 0x9e6495a3,
        0x0edb8832, 0x79dcb8a4, 0xe0d5e91e, 0x97d2d988,
        0x09b64c2b, 0x7eb17cbd, 0xe7b82d07, 0x90bf1d91,
        0x1db71064, 0x6ab020f2, 0xf3b97148, 0x84be41de,
        0x1adad47d, 0x6ddde4eb, 0xf4d4b551, 0x83d385c7,
        0x136c9856, 0x646ba8c0, 0xfd62f97a, 0x8a65c9ec,
        0x14015c4f, 0x63066cd9, 0xfa0f3d63, 0x8d080df5,
        0x3b6e20c8, 0x4c69105e, 0xd56041e4, 0xa2677172,
        0x3c03e4d1, 0x4b04d447, 0xd20d85fd, 0xa50ab56b,
        0x35b5a8fa, 0x42b2986c, 0xdbbbc9d6, 0xacbcf940,
        0x32d86ce3, 0x45df5c75, 0xdcd60dcf, 0xabd13d59,
        0x26d930ac, 0x51de003a, 0xc8d75180, 0xbfd06116,
        0x21b4f4b5, 0x56b3c423, 0xcfba9599, 0xb8bda50f,
        0x2802b89e, 0x5f058808, 0xc60cd9b2, 0xb10be924,
        0x2f6f7c87, 0x58684c11, 0xc1611dab, 0xb6662d3d,
        0x76dc4190, 0x01db7106, 0x98d220bc, 0xefd5102a,
        0x71b18589, 0x06b6b51f, 0x9fbfe4a5, 0xe8b8d433,
        0x7807c9a2, 0x0f00f934, 0x9609a88e, 0xe10e9818,
        0x7f6a0dbb, 0x086d3d2d, 0x91646c97, 0xe6635c01,
        0x6b6b51f4, 0x1c6c6162, 0x856530d8, 0xf262004e,
        0x6c0695ed, 0x1b01a57b, 0x8208f4c1, 0xf50fc457,
        0x65b0d9c6, 0x12b7e950, 0x8bbeb8ea, 0xfcb9887c,
        0x62dd1ddf, 0x15da2d49, 0x8cd37cf3, 0xfbd44c65,
        0x4db26158, 0x3ab551ce, 0xa3bc0074, 0xd4bb30e2,
        0x4adfa541, 0x3dd895d7, 0xa4d1c46d, 0xd3d6f4fb,
        0x4369e96a, 0x346ed9fc, 0xad678846, 0xda60b8d0,
        0x44042d73, 0x33031de5, 0xaa0a4c5f, 0xdd0d7cc9,
        0x5005713c, 0x270241aa, 0xbe0b1010, 0xc90c2086,
        0x5768b525, 0x206f85b3, 0xb966d409, 0xce61e49f,
        0x5edef90e, 0x29d9c998, 0xb0d09822, 0xc7d7a8b4,
        0x59b33d17, 0x2eb40d81, 0xb7bd5c3b, 0xc0ba6cad,
        0xedb88320, 0x9abfb3b6, 0x03b6e20c, 0x74b1d29a,
        0xead54739, 0x9dd277af, 0x04db2615, 0x73dc1683,
        0xe3630b12, 0x94643b84, 0x0d6d6a3e, 0x7a6a5aa8,
        0xe40ecf0b, 0x9309ff9d, 0x0a00ae27, 0x7d079eb1,
        0xf00f9344, 0x8708a3d2, 0x1e01f268, 0x6906c2fe,
        0xf762575d, 0x806567cb, 0x196c3671, 0x6e6b06e7,
        0xfed41b76, 0x89d32be0, 0x10da7a5a, 0x67dd4acc,
        0xf9b9df6f, 0x8ebeeff9, 0x17b7be43, 0x60b08ed5,
        0xd6d6a3e8, 0xa1d1937e, 0x38d8c2c4, 0x4fdff252,
        0xd1bb67f1, 0xa6bc5767, 0x3fb506dd, 0x48b2364b,
        0xd80d2bda, 0xaf0a1b4c, 0x36034af6, 0x41047a60,
        0xdf60efc3, 0xa867df55, 0x316e8eef, 0x4669be79,
        0xcb61b38c, 0xbc66831a, 0x256fd2a0, 0x5268e236,
        0xcc0c7795, 0xbb0b4703, 0x220216b9, 0x5505262f,
        0xc5ba3bbe, 0xb2bd0b28, 0x2bb45a92, 0x5cb36a04,
        0xc2d7ffa7, 0xb5d0cf31, 0x2cd99e8b, 0x5bdeae1d,
        0x9b64c2b0, 0xec63f226, 0x756aa39c, 0x026d930a,
        0x9c0906a9, 0xeb0e363f, 0x72076785, 0x05005713,
        0x95bf4a82, 0xe2b87a14, 0x7bb12bae, 0x0cb61b38,
        0x92d28e9b, 0xe5d5be0d, 0x7cdcefb7, 0x0bdbdf21,
        0x86d3d2d4, 0xf1d4e242, 0x68ddb3f8, 0x1fda836e,
        0x81be16cd, 0xf6b9265b, 0x6fb077e1, 0x18b74777,
        0x88085ae6, 0xff0f6a70, 0x66063bca, 0x11010b5c,
        0x8f659eff, 0xf862ae69, 0x616bffd3, 0x166ccf45,
        0xa00ae278, 0xd70dd2ee, 0x4e048354, 0x3903b3c2,
        0xa7672661, 0xd06016f7, 0x4969474d, 0x3e6e77db,
        0xaed16a4a, 0xd9d65adc, 0x40df0b66, 0x37d83bf0,
        0xa9bcae53, 0xdebb9ec5, 0x47b2cf7f, 0x30b5ffe9,
        0xbdbdf21c, 0xcabac28a, 0x53b39330, 0x24b4a3a6,
        0xbad03605, 0xcdd70693, 0x54de5729, 0x23d967bf,
        0xb3667a2e, 0xc4614ab8, 0x5d681b02, 0x2a6f2b94,
        0xb40bbe37, 0xc30c8ea1, 0x5a05df1b, 0x2d02ef8d
    };

    static void crc32_step(struct crc32_context *context, uint8_t byte)
    {
       context->crc = (context->crc >> 8) ^ CRC32Table[(context->crc ^ (uint32_t)byte) & 0x000000FFul];
    }

    static uint32_t crc32_finalize(struct crc32_context *context)
    {
        return context->crc;
    }

  #elif // MIN_TRANSPORT_USE_CRC_TABLE

    static void crc32_step(struct crc32_context *context, uint8_t byte)
    {
        context->crc ^= byte;
        for(uint32_t j = 0; j < 8; j++) {
            uint32_t mask = (uint32_t) -(context->crc & 1U);
            context->crc = (context->crc >> 1) ^ (0xedb88320U & mask);
        }
    }

    static uint32_t crc32_finalize(struct crc32_context *context)
    {
        return ~context->crc;
    }

  #endif // MIN_TRANSPORT_USE_CRC_TABLE

#endif // MIN_TRANSPORT_PROTOCOL
kentindell commented 6 years ago

I'm going to revisit the CRC issue completely after discovering the research that Koopman did at CMU. I had originally used Fletcher's Checksum, which was very cheap to compute. But I also need to put an individual CRC for just the header (8-bit CRC should be OK) and then a CRC for the rest of the payload. But the CRC polynomial should be picked for a particular Hamming distance requirement: not all polynomials are the same.

As well as the above, CRC hardware is becoming quite common in MCUs so the software should allow the CRC implementation to be "plugged", which would allow CRC hardware to be driven, or a lookup table implementation, or a CPU-intensive one, all depending on the requirements of the target device. I do want MIN to be workable for small AVR or PIC devices, which don't have the space for a lookup table or have CRC hardware. So the 'driver API' model is appropriate for this.

erakis commented 6 years ago

I'm going to revisit the CRC issue completely after discovering

Great, anyway this is not a urgent concern for me....

Suggestion 4

As you probably guest, I'm using the target source code version for Visual Studio 2015 compiler and GCC 4.9. I need to use it on small linux single board and also on demo application running on Windows Desktop. I don't want to use Pyton for desktop version and converting all your works to C++ would be my last options.

So... to be able to use min-protocol in a C++ environment, it would be more flexible to pass a void *ext instead of a uint8_t port. This way we could use the ext pointer to jump to a c++ class member and on a specific instance. Ex :

#include <cstdint>
#include <vector>

extern "C"
{
    #include <min.h>
}

class MySerialPort
{
public:
    void doSomething();
    void doSomethingMore();

    void init()
    {
        min_init_context(&ctx, static_cast<void *>(this));
    }

    min_context ctx;
    std::vector<uint8_t> txBuffer;
}

void min_tx_start(void* ext)
{
    /* ext will have been passed to min_init_context and pass back to all min callback 
       method to allow dereferencing a MySerialPort instance. Otherwise I would have 
       to declare as much buffer as I will have possible instance of serial 
       port (MySerialPort). */
    MySerialPort* my = static_cast<MySerialPort *>(ext);
    my.txBuffer.clear();
}

Question

I'm not sure to understand why you test the value of remoteConnected on this line https://github.com/min-protocol/min/blob/master/target/min.c#L588. Why not keep sending the the last frame for ever or until we get a reset ? By the way, I create a server/client test suite and your protocol implementation rock, I've test a lot of synchronization potential problem and each time the synchronization come back.

kentindell commented 6 years ago

I'll look at making that change to support C++: it's a good idea.

To answer the question, I did used to just resend the data but it screws up some systems that buffer the data at the receiver end even if the app consuming the data has stopped (I had trouble with Windows serial and USB).

erakis commented 6 years ago

Thank you for your answer. For now I'm re-implementing a C++ version as I need it right now.

Bug ?

Here https://github.com/min-protocol/min/blob/master/target/min.c#L152, we should not read 

assert(elf->transport_fifo.n_frames != 0);

instead of 

assert(n_frames != 0);

?

kentindell commented 6 years ago

Yeah, looks like a bug. Weird: that wouldn't even compile yet I'm pretty sure the Arduino build testing was done with assertion checking turned on.

erakis commented 6 years ago

Sorry to disturb you again.

Compilation Error

static void crc32_step(struct crc32_context *context, uint8_t byte)
{
    context->crc ^= byte;
    for(uint32_t j = 0; j < 8; j++) {
        uint32_t mask = (uint32_t) -(context->crc & 1U);       <-- THIS LINE
        context->crc = (context->crc >> 1) ^ (0xedb88320U & mask);
    }
}

On Visual Studio 2015, I'm getting this compilation error

C4146   unary minus operator applied to unsigned type, result still unsigned

Which could be fix by

static void crc32_step(struct crc32_context *context, uint8_t byte)
{
    context->crc ^= byte;
    for(uint32_t j = 0; j < 8; j++) {
        context->crc = (context->crc >> 1) ^ (context->crc & 1)* 0xedb88320U;
    }
}
kentindell commented 6 years ago

I don't think that's a C error, it's just a weird aspect of the language worthy of a warning. It does compile efficiently on a small MCU to a simple 2's complement operation.

But in any case I am going to revisit all the CRC code anyway: I don't think 32 bits is required and a good polynomial for a 24-bit CRC should work just as well (maybe the one FlexRay uses).