Git Product home page Git Product logo

Comments (11)

kentindell avatar kentindell commented on August 11, 2024

Those look like good suggestions.

from min.

erakis avatar erakis commented on August 11, 2024

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

  • Compilation bug
  • Suggestion 1

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

from min.

kentindell avatar kentindell commented on August 11, 2024

Done all three fixes: 4626d73

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

from min.

erakis avatar erakis commented on August 11, 2024

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

from min.

kentindell avatar kentindell commented on August 11, 2024

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.

from min.

erakis avatar erakis commented on August 11, 2024

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.

from min.

kentindell avatar kentindell commented on August 11, 2024

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).

from min.

erakis avatar erakis commented on August 11, 2024

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);

?

from min.

kentindell avatar kentindell commented on August 11, 2024

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.

from min.

erakis avatar erakis commented on August 11, 2024

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;
    }
}

from min.

kentindell avatar kentindell commented on August 11, 2024

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).

from min.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.