Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[UNRESOLVED] -- Keeping CAN (Control Area Network) packet transmit order #4090

Closed
thomasonw opened this issue Mar 31, 2017 · 18 comments
Closed

Comments

@thomasonw
Copy link

Description

  • Type: Enhancement

There are a class of CAN messages which are transmitted multi-packet. Some protocols include pack numbers, to allow recreation of the transmission in the event of out-of-order packets. However, other protocols depend on the packet order to be retained. A specific example of the latter is FastPacket messages in NMEA2000

Currently the MBED API has no way to address packet orders, either by specifying packet Tx must retain their order, or by allowing an app to check if the Tx buffer(s) has cleared before attempting to send the next packet.

I would like to ask how to proceed. Some ideas include:

  1. MBED adopting the standard that CAN message packet order is always retained (ala, like socketCAN in Linux).
  2. Allow an API to optionally specify this capability, say in init() / open() or a new additional API
  3. Allow an API to query the status of the Tx buffers, returning if they are all empty or not.
  4. ???

At present I am coding manually directly to the CPU (ala, the STM32F0 -- setting the TXFP bit true to force FIFO, but this clearly is not the MBED way.)

Any of these changes would require agreement from all and modification of the existing CPUs drivers - my preference would be option #1, as it does not change the API, only the drivers, and there are few cases where a true FIFO will not work well in CAN systems.

@chrissnow
Copy link
Contributor

Interested to hear about your NMEA2000 project, I also ported this
https://github.com/ttlappalainen/NMEA2000 to mbed and got it working ok, haven't tried any FastPacket though and it was on a LPC1768.

@thomasonw
Copy link
Author

@chrissnow : Yes, that is the same lib. We have been working on making it more universal, and including now MBED. (See his latest versions, and here for a start at the MBED 'bridge' https://github.com/thomasonw/NMEA2000_mbed )

I am targeting the STM32F line of CPUs, would be a good test to check the MBED port out on the LPC line as well once get a few more details worked out. Any suggestions on passing port (Tx/Rx pins) as well as serial stream?

And do you know if the LPC1768 (with its driver) keeps packet send order?

@kentindell
Copy link

"there are few cases where a true FIFO will not work well in CAN systems"

You've not heard of unbounded priority inversion? It's crucial for real-time performance on a heavily-loaded bus that frames are internally arbitrated by ID: if not the worst-case latencies grow hugely.

@thomasonw
Copy link
Author

@kentindell, Yes, you are correct - for heavily loaded CAN systems. But I will put forward the simple CAN API in MBED are not appropriate for such an environment. As bus utilization increased the management of the Tx mailboxes need much more control, including much better software Tx queue management (multiple software Tx queues not being uncommon) in the driver as well as closer coupling to the hardware.

@thomasonw
Copy link
Author

This is a follow on to #4359 (and #4121), as it seems more appropriate to document here.

@0xc0170 - Hello. I am reaching out to you as we are getting to a point where we need to make an architectural decision, and need to understand the intention of MBED to address the inability to assure packet order in the current CAN APIs.

As a review: there are a class of messages in NMEA2000 which by their nature require packet order be kept during transmission. Presently the MBED API does not allow for us to assure packet order is kept (not only assuring queues are true FIFO, but also that any Tx retries are completed before the next packet is scheduled for transmission).

To date we have been using a patched MBED library during development, but clearly this is not a desirable way to more forward. Which is why I am asking: what is the intention of the MBED team to address this issue?

@kentindell
Copy link

In my own CAN drivers for the ST bxCAN I run a software priority queue of up to 64 frames (using a word mask and first-set bit instructions, similar to how many RTOS task management systems work).

If two frames are queued with the same arbitration ID/IDE values then it is indeterminate which ones gets sent first. This is exactly the same issue with CAN controllers that have direct priority queues in hardware (e.g. the Atmel MCAN controller). My API includes a status query call that lets the caller ask if any given frame has been transmitted on the bus. It also includes a callback function that notifies the application of frame transmission so that polling can be avoided if necessary (the API allows for application-specific 'tags' to be associated with a frame which can be used in the application for specific frame handling). This maps well to a number of CAN controller hardware designs (including the Atmel one). Using this mechanism I support configurable FIFO queues feeding into the overall priority queue.

FIFO queueing across all CAN frames in a device is ruinous for real-time latencies. It's all very well saying the bus load won't be high but (a) this never stays true for long over a project's lifetime, and (b) there are always intermittent periods of high bus load - but they may not happen frequently. Having only a FIFO CAN driver induces priority inversion which then means chasing highly intermittent timing problems - that become Heisenbugs with all the unpleasant consequences when they make it into a deployed system.

I should also add that appealing to the SocketCAN FIFO approach is ironic: SocketCAN randomly re-orders received frames from the bus - quite an achievement!

@thomasonw
Copy link
Author

Hello. I was going to close this issue as unresolved, but before doing so wanted to check one more time. @0xc0170 @theotherjimmy you two seems to perhaps be the most authoritative: Is there any intention for MBED to address the current CAN API deficiency preventing implementation of NMEA2000? We would like to support MBED, but need to proceed forward (And we do have alternatives, just not MBED compatible). Looking forward to hearing guidance.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 2, 2017

cc @sg- @bulislaw

@thomasonw thomasonw changed the title Keeping CAN (Control Area Network) packet transmit order [UNRESOLVED] -- Keeping CAN (Control Area Network) packet transmit order Nov 25, 2017
@thomasonw
Copy link
Author

Closing this due to lack of progress on resolution. MBED is unable to support NMEA2000 deployments without using priority CAN drivers and/or custom patched libraries.

@dfreiberger
Copy link

@0xc0170 The inability to enable chronological ordering of the CAN FIFO is also posing an issue in our application, where we are relying on ISO-TP for certain application functions. ISO-TP (see https://en.wikipedia.org/wiki/ISO_15765-2) relies on segmented transfer, and as such it breaks if messages are sent out of order.

Exposing a way to enable Transmit FIFO Priority through the CAN interface (such as is proposed in https://github.com/ARMmbed/mbed-os/pull/4359/commits) in STM32F4xx would resolve the issue for us.

Is there any intent to review this issue again in the future, or should we find another way around this issue?

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 4, 2019

Exposing a way to enable Transmit FIFO Priority through the CAN interface (such as is proposed in https://github.com/ARMmbed/mbed-os/pull/4359/commits) in STM32F4xx would resolve the issue for us.

If we can make sure this is generic enough, please create PR and resume the discussion.

@ARMmbed/team-st-mcd To be aware of this, would you be able to help?

@jeromecoutant
Copy link
Collaborator

Hi

I have quickly discussed with a CAN driver expert,
and, as requested by @dfreiberger, exposing a way to enable Transmit FIFO Priority through the CAN interface seems to be a good point for mbed-os
It's even mandatory for protocol like "NMEA2000" as explained by @thomasonw

But proposed patch seems not correct as the transmit priority is not a mode.
CAN API should add a new parameter, where default value could be the current one.

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 4, 2019

@jeromecoutant Quick good feedback ! Would you create this new parameter addition?

Based on the interest, few users would benefit from enabling this on STM boards.

@kentindell
Copy link

You can resolve this by providing a “Frame transmitted” callback or similar mechanism so that a FIFO can be built on top of the API: frames are put into the FIFO and then taken out in response to a frame being transmitted. This avoids unbounded priority inversion in the main system but preserves FIFO ordering for the applications that need it.

@jeromecoutant
Copy link
Collaborator

@0xc0170 to be honest, I think you should not waiting for us for this API update...

@dfreiberger
Copy link

I was able to resolve this for our purposes by extending the CAN class and configuring TXFP ByRequestOrder for our microcontroller. In general I think it would be good to support this in the mbed CAN implementation, but I am not familiar enough with the stack to propose a solution.

enum FifoPriority {
        ByIdentifier,
        ByRequestOrder
};

int ExtendedCAN::fifo_priority(FifoPriority priority)
{
    lock();

    int success = 0;
    CAN_TypeDef *can = _can.CanHandle.Instance;

    can->MCR |= CAN_MCR_INRQ ;
    while ((can->MSR & CAN_MSR_INAK) != CAN_MSR_INAK) {
    }

    switch (priority) {
        case ByIdentifier:
            can->MCR &= ~(uint32_t)CAN_MCR_TXFP;
            success = 1;
            break;
        case ByRequestOrder:
            can->MCR |= CAN_MCR_TXFP;
            success = 1;
            break;
        default:
            success = 0;
            break;
    }

    can->MCR &= ~(uint32_t)CAN_MCR_INRQ;
    while ((can->MSR & CAN_MSR_INAK) == CAN_MSR_INAK) {
    }

    return success;

    unlock();
}

@jeromecoutant
Copy link
Collaborator

@dfreiberger I suggest you to raise a new issue (or better raise a pull request :-))

@0xc0170 How can we address this ?

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 24, 2019

Not certain we can't do anything here neither. reopening this issue would not help as it has not been fixed since 2017.

Patches welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants