Bit timing config for PCAN

[christiansandberg]

Depends on #615 to be merged first.
Fixes #538.
Part of #614.

[codecov]

Codecov Report

Merging #625 (22dc9e3) into develop (6c01b3c) will decrease coverage by 0.24%.
The diff coverage is 2.77%.

@@             Coverage Diff             @@
##           develop     #625      +/-   ##
===========================================
- Coverage    70.41%   70.17%   -0.25%     
===========================================
  Files           72       72              
  Lines         7123     7148      +25     
===========================================
  Hits          5016     5016              
- Misses        2107     2132      +25     

[codecov]

Codecov Report

Merging #625 into develop will increase coverage by 1.68%.
The diff coverage is 2.85%.

@@             Coverage Diff             @@
##           develop     #625      +/-   ##
===========================================
+ Coverage    61.99%   63.67%   +1.68%     
===========================================
  Files           63       63              
  Lines         5623     5644      +21     
===========================================
+ Hits          3486     3594     +108     
+ Misses        2137     2050      -87

[christiansandberg]

@bmeisels, could you have a look at this and tell me what you think?

[bmeisels]

@christiansandberg, I've been following your work on #615 and it looks very promising. See my note about unifying arbitration and data timings.

As a general note I think my parameter list parsing is a bit cleaner compared to the change you made but as this will be removed in the future I think this is more of a style issue. I still think there is some cleanup that can be done.

Also it seems you dropped support for f_clock_mhz when using the timing class. For PCAN i don't think this matters but I am not sure about other interfaces.

I will try and test the code on my setup later this week.

Keep up the good work!

[christiansandberg]

See my note about unifying arbitration and data timings.

I must have missed that note, where is it?

As a general note I think my parameter list parsing is a bit cleaner compared to the change you made but as this will be removed in the future I think this is more of a style issue. I still think there is some cleanup that can be done.

I agree. I just wanted to gather the deprecated code to one place and make it match with the class section.

Also it seems you dropped support for f_clock_mhz when using the timing class. For PCAN i don't think this matters but I am not sure about other interfaces.

It is just a convenience parameter so I thought it might add more confusion to have multiple ways of specifying the frequency. SI-units should be preferred.

[bmeisels]

The note I mentioned is on the PCAN code in the PR. You can see it above in this page.

[christiansandberg]

Have you submitted the review?

[bmeisels]

I think there should be a single timing object which includes both arbitration and data timings.
This also solves the problem of multiple f_clock settings.

[christiansandberg]

If you mean that the class should handle both arbitration and data timings I'm not sure I agree about that. Apart from f_clock (which is not always required), everything else would have to be duplicated or require some overly complex code structures in that class. There would also need to be separate classes for CAN-FD and regular CAN to not add confusion for most users which do not use CAN-FD.

This solution does not need to consider if the timing settings are used for normal CAN, arbitration phase or data phase since they are basically the same and have no dependency apart from clock frequency and can therefore be specified separately.

[bmeisels]

I don't see why this will complicate the usage as long as the data phase timings are optional.

Just to make sure we share the same understanding. Normal timings aka nom are used for both arbitration and data in standard CAN. Data timings are only used in CAN-FD for the data phase.

[christiansandberg]

For standard CAN there is no nominal or arbitration or data timing, just timing. I don’t think it makes sense to force CAN-FD nomenclature on standard CAN users, especially when the interfaces don’t even support CAN-FD.

[bmeisels]

I don't agree. The data timing are only used when the bit rate switch flag is marked. This means that the nominal bit timings have generally the same meaning as in classic CAN (and technically can be used with classic CAN on the same bus as long as the CAN ICs are FD bus compatible, even if they don't support the FD extensions). The data timings are the addition. The Peak software tools seem to treat the timings as I have described.

I don't see how this is forcing anything on classic CAN users. If you think the naming is clearer the way it is now I would add the data timings and not add the nominal prefix to the existing timings.

[christiansandberg]

How about this? Since timings are then grouped by prefix there is no added value for the user to have a separate class for specifying timing values. They can enter them into the Bus constructor directly. Internally we can construct BitTiming instances however using some utility function taking a bunch of kwargs and returning a nominal and data timing objects.

[bmeisels]

I must have missed it somehow.

[felixdivo]

#615 is now merged.

[felixdivo]

@christiansandberg Can this now be finalized?

[christiansandberg]

I would prefer to get some more input on how we want to have it first. Basically if we should use class instances to divide between normal and data phase timing or use prefixes (like now).

[felixdivo]

I am in favour of using a seperate class, since that one could (a) do common validation on the parameters that would else need to be duplicated in some way and (b) it factors out a common group of parameters, making the construction of a bus instance more readable.

I also like the way timing is separate from data_timing, it serves the intuition that the data phase has different timing parameters and keeps things relatively simple.

The one restriction that f_clock is duplicated can be solved by a simple if-raise block, I don't see a problem there. That's a good compromise from my point of view.

But one thing is important; this should be handled the same way in all interfaces that use this new class, and we could probably add a not on this in the BitTiming class.

[bmeisels]

@felixdivo @christiansandberg
I still think we should consider adding a FDBitTiming class which will include both configurations.
This would also allow us to remove the FD parameter as we could just check if the timing is a FD timing.
I think this is much better than passing 2 timing object (like in #650) or only constructing them internally (as proposed by @christiansandberg).

[christiansandberg]

Maybe it is overkill exposing a class for this in the API. How about using dictionaries to group the timing parameters but having possibility to pass f_clock separately since it is common for both? Also the bitrate could be taken from the kwargs since that is the standard way to pass it when not using timing parameters.

# Specifying all parameters in the timing dictionary
can20_bus = can.Bus("channel", "interface",
    timing={bitrate=1000000, f_clock=8000000, tseg1=5, tseg2=2, sjw=1}
)

# Or specifying bitrate and f_clock in kwargs
can20_bus = can.Bus("channel", "interface",
    bitrate=1000000,
    f_clock=8000000,
    timing={tseg1=5, tseg2=2, sjw=1}
)

# CAN-FD buses also needs a data_timing dict for CAN-FD frames only while regular frames are unaffected
canfd_bus = can.Bus("channel", "interface",
    f_clock=80000000,
    timing={bitrate=500000, tseg1=119, tseg2=40, sjw=40},
    data_timing={bitrate=2000000, tseg1=29, tseg2=10, sjw=10}
)

Those will internally be constructed to BitTiming classes, using some utility function that all interfaces can use.

# A util function for constructing a BitTiming object
def get_bit_timing_object(timing=None, f_clock=None, bitrate=None):
    timing_args = dict(timing or {})
    timing_args.setdefault("bitrate", bitrate)
    timing_args.setdefault("f_clock", f_clock)
    return can.BitTiming(**timing_args)


class SomeBus:
    def __init__(self, bitrate=None, f_clock=None, timing=None, data_timing=None):
        # Create a timing object from kwargs
        timing_obj = get_bit_timing_object(timing, f_clock, bitrate)
        # Optionally add CAN-FD data timing as well
        data_timing_obj = get_bit_timing_object(data_timing, f_clock)

[bmeisels]

Had a look at this again.
I see 2 options:

1. Extend the bit timing class with optional timings for CAN FD. This will allow using a single class which can be passed around and should be transparent to users of standard CAN.
2. Have 2 instances of timing and make sure to test that the f_clock is the same on both.

I strongly prefer 1 and would like to hear other opinions.

[christiansandberg]

I'd like to add a third option, which is for the user to pass all bit timing settings as individual keyword arguments just like they do today, but with standardized names (and with today's names as deprecated aliases). If you prefer to group the settings in one or more objects to pass around you can still do that using dictionaries and the **dict syntax to pass them on to the constructor.

The interface implementations could just pass their **kwargs to some helper function which returns BitTiming objects which can be used internally.

One reason I would not prefer option 1 is to keep the code DRY. All calculations would have to be duplicated although they do the same thing. Bit timing calculations are independent of whether it is for CAN, CAN-FD, arbitration or data.

[felixdivo]

This is tagged to be included in Version 4.0, is this close to being merged? It seems like there isn't too much missing.

[bmeisels]

@felixdivo @christiansandberg I think the third option should be ok.
This would mean changing most of this PR.

I don't currently have time / convenient access to hardware on a daily basis but if someone spun up another version I would try to test it.

@christiansandberg what do you think?

[felixdivo]

the third option should be ok

Agreed. However, I haven't touched the PCAN or CAN FD stuff so I'd rather not change anything there.

[christiansandberg]

I'll start doing some implementation but I don't have any hardware access either.

[jan-bruckner]

I could do some testing with PCAN-USB FD adapters, just let me know :)

[zariiii9003]

Superseded by #1514

[felixdivo]

@zariiii9003 Should we keep this branch?

[zariiii9003]

@felixdivo deleted. I usually don't delete branches of other maintainers 😄

[felixdivo]

Thanks!

I'm also reluctant to do so, but I also think that fewer branches help others to maintain on overview over the project. 🙂 And besides, the diff is preserved in the PR.