Skip to content

Kvaser single_handle mode issues? #70

Description

@hardbyte

Originally reported by: Christian Sandberg (Bitbucket: sandberg, GitHub: sandberg)


I'm not sure if these are actual issues or if I just don't understand.

  1. shutdown() doesn't close the _read_handle.
  2. The writing_event doesn't seem to be set anywhere, is it supposed to be done in send()?
  3. send() doesn't wait to access the bus if a read operation is in progress.
  4. pc_time_offset is never used when calculating timestamp. I assume that we want timestamp to be in unix time? Right now the first message has timestamp 0.
  5. Flags are not calculated on send(), i.e. extended messages can not be sent.
  6. is_remote_frame and is_error_frame is not set on recv().
  7. channel_info is not changed (suggest to set it to the Kvaser product name and channel)

Activity

  1. hardbyte commented on Jul 16, 2016

    @hardbyte
    OwnerAuthor

    Original comment by Christian Sandberg (Bitbucket: sandberg, GitHub: sandberg):


    If the purpose of the Event and Condition is to avoid simultaneous access to the handle then wouldn't it suffice to acquire a Lock() in both recv() and send() when the APIs are called?

  2. hardbyte commented on Jul 18, 2016

    @hardbyte
    OwnerAuthor

    Original comment by Brian Thorne (Bitbucket: hardbyte, GitHub: hardbyte):


    1. I'm unsure about the need to explicitly go bus off in either case. As it looks I'd say that shutdown should go bus off on the read handle. I'll add that now.

    2 was a fun trip in software archaeology - here is where it was set in 2012 - I think the event and condition were the correct design for the problem though. From memory the problem was trying to read as fast as the kvaser device would allow, except when trying to send a message which would then be given priority of the bus.

    3 - what can happen?

    I think 2 and 3 point out clearly that the threading "protection" is currently broken - want to put together a PR? I would't like to hack at it too much myself as I don't have access to a kvaser anymore. But more than happy to help with reviewing etc.

  3. hardbyte commented on Jul 19, 2016

    @hardbyte
    OwnerAuthor

    Original comment by Christian Sandberg (Bitbucket: sandberg, GitHub: sandberg):


    1. Looks good.

    2. I see. Seems to be some more dead code left from that time. In my opinion whenever you want to send and receive using different threads, you should use two handles (as per Kvaser's instructions). However, if the interface is accessed only from one thread, one could use single_handle mode. Therefore I think using locks is not really necessary on this level. The developer could make that decision and if he wants to access the same handle using different threads, he should use appropriate locks on a higher level to prevent concurrent access.

    3. Not sure what would happen but as Kvaser writes, you should not access the same handle simultaneously in different threads. Now one could send during a read operation (even if it is only 1 ms).

    My suggestion is to clean out all threading related things here and instead update the documentation about using a single handle from different threads. Alternatively, acquire a lock in both recv() and send() so that they can never be accessed at the same time. What do you think? I'm happy to create a pull request.

  4. hardbyte commented on Jul 19, 2016

    @hardbyte
    OwnerAuthor

    Original comment by Christian Sandberg (Bitbucket: sandberg, GitHub: sandberg):


    Updated with nr 4.

  5. hardbyte commented on Jul 20, 2016

    @hardbyte
    OwnerAuthor

    Original comment by Christian Sandberg (Bitbucket: sandberg, GitHub: sandberg):


    Added 5, 6, and 7.

  6. hardbyte commented on Aug 2, 2016

    @hardbyte
    OwnerAuthor

    Original comment by Brian Thorne (Bitbucket: hardbyte, GitHub: hardbyte):


    Various fixes for Kvaser.

    Explicitly convert channel to integer.
    Remove threading related stuff.
    Fix timestamps.
    Fix flags not being calculated on transmission.
    Add channel info.
    Reduced timestamp resolution to 10 us to reduce risk of overflow.
    Tweaked logging levels.
    Closes #47.
    Closes #70.

  7. added 2 commits that reference this issue on Dec 2, 2016
    339d4ea
    de8eed1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions