Skip to content

Add ETAS interface#1144

Merged
felixdivo merged 9 commits intohardbyte:developfrom
felixn:etas
Dec 16, 2021
Merged

Add ETAS interface#1144
felixdivo merged 9 commits intohardbyte:developfrom
felixn:etas

Conversation

@felixn
Copy link

@felixn felixn commented Oct 25, 2021

This patch adds support for ETAS CAN Interfaces, based on the ETAS BOA (Basic Open API)

It's ready from a functional point of view.
I'm not sure what else is required for merging - @felixdivo, what do you think?
I tried to follow the steps described in https://github.com/hardbyte/python-can/blob/develop/doc/development.rst#creating-a-new-interfacebackend as well as #1149

I've added a back2back test, which mostly passes - but it can obviously only run successfully if an actual hardware interface is available.
b2b

  • test_timestamp_is_absolute fails because timestamps are HW timestamps relative to powerup, not relative to unix epoch. What's the expected behaviour?
  • test_unique_message_instances fails because the API does not seem to allow creating multiple instances of one channel with differing settings regarding self-reception...

Are further tests, e.g. unit tests with mocked API/HW access, mandatory?

@mergify mergify bot requested a review from hardbyte October 25, 2021 18:14
@codecov
Copy link

codecov bot commented Oct 26, 2021

Codecov Report

Merging #1144 (5ff0f15) into develop (cee2d78) will decrease coverage by 3.18%.
The diff coverage is 3.50%.

@@             Coverage Diff             @@
##           develop    #1144      +/-   ##
===========================================
- Coverage    68.43%   65.24%   -3.19%     
===========================================
  Files           84       86       +2     
  Lines         8418     8846     +428     
===========================================
+ Hits          5761     5772      +11     
- Misses        2657     3074     +417     

@felixn felixn changed the title WIP: add ETAS interface Add ETAS interface Nov 6, 2021
@felixdivo
Copy link
Collaborator

Hey @felixn, Felix here. ^^

At first glance, your PR looks very good! Mandatory is working code and proper documentation, of which you seem to have included both (I can't test the code since I do not have the required hardware). Further tests are not required.

Regarding the timestamp: According to the docs, they are relative to "epoch in seconds". For example, the pcan interface also translates timestamps like this. Maybe one could move that piece of code to can/util.py and you can use it too? Could you do that?

Regarding the last test case: It's totally fine to skip the test case if that is not supported, just add this to your test case class (you can of course change the description as you see fit):

def test_unique_message_instances(self):
    self.skipTest("Creating the test environment with additional buses is not supported")

@felixdivo
Copy link
Collaborator

If you choose to implement the suggested timestamp fix, you'd also need to add a extension key to setup.py and advice users to install the entire library with pip install can[etas] (or something like that).

@felixn
Copy link
Author

felixn commented Nov 11, 2021

Hi Felix ;)
Thanks for the quick feedback!

I've made changes you suggested to make the timestamps relative to epoch.
Luckily, I didn't need the additional uptime library, since in this case the timestamps are relative to a hardware timer.

etas

Ready to merge from my point of view!

Copy link
Collaborator

@felixdivo felixdivo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I went through your code line-by-line (except for can/interfaces/etas/boa.py).

This is a well-written contribution! Thank you!

The changes I mentioned are all rather small (I hope), except for the errcheck idea. What do you think?

By the way: A final review by @hardbyte is also required, only then this will be merged.

@felixn felixn requested a review from felixdivo November 14, 2021 18:10
@felixn
Copy link
Author

felixn commented Nov 14, 2021

Thanks for the feedback @felixdivo - all valuable, I made the changes you requested.
@hardbyte - what do you think?

Copy link
Collaborator

@felixdivo felixdivo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@nbusser
Copy link
Contributor

nbusser commented Nov 19, 2021

Thank you for your interest in ETAS driver.
For your information, PR #914 has been opened last year by @vincent-mailhol, the writer of the driver.
Maybe there are also some interesting ideas there

@vincent-mailhol
Copy link

@nbusser, thanks for the mention. Indeed, Philipp Ahrendt and myself also implemented all the bindings for the ETAS interfaces. However, I never implemented the mock-up test thus my pull request failed the auto checks. Since then, I moved to some other projects. I do not think I will go back on this one day. None the less, what we did was tested with a wide variety of ETAS hardware (including the virtual bus) to make sure to capture all the edge cases.
@felixn thanks for your work. I would really be happy to see the ETAS interface merged to python CAN. I also recommend you to have a look at what we did. We did our best to cover all the use cases such as the older API (BOA version 1.0.0 to 1.4.0), the bus errors (Error active, passive, bus off…). We also added an interface for the optional bit timing parameters (time segment, Synchronization Jump Width…)

@felixn
Copy link
Author

felixn commented Nov 22, 2021

@vincent-mailhol & @nbusser :
Thanks for connecting the dots!
Still, I must say that this is a rather frustrating situation - discovering code in pull requests is next to impossible, and a lot of effort could have been saved...

I see several good ideas from your code that I could adopt - as you mentioned.
However, the code as it is right now is functional and a big improvement from "no support at all".
@hardbyte - are you ok with merging this as-is, and extending in a second step?
I can't justify spending the effort to extend the implementation for more use cases without some kind of assurance the code will be considered for merging.

@felixdivo
Copy link
Collaborator

@hardbyte - are you ok with merging this as-is, and extending in a second step?

I'm not him, but I'd say it's fine to merge it like this. However, improving the documentation with the small suggestion by @vincent-mailhol would still be useful in this PR. Afterwards, I'd personally say it's ready to be merged.

@hardbyte
Copy link
Owner

hardbyte commented Dec 6, 2021

@hardbyte - are you ok with merging this as-is, and extending in a second step?

I'm not him, but I'd say it's fine to merge it like this. However, improving the documentation with the small suggestion by @vincent-mailhol would still be useful in this PR. Afterwards, I'd personally say it's ready to be merged.

@felixn thanks for your contribution. I understand the frustration and uncertainty, I'm really disappointed when I see changes not make it in and also when efforts are duplicated. It's at least partly my fault for trying to stay across all core changes - despite the fact I haven't used CAN professionally in about 7 years(!)

I try to ensure we only merge clear and maintainable code but I'm (obviously) not able to keep up. I plan to make a few tweaks over my Christmas break primarily to ensure things are more sustainable. Long story short, as long as a core contributor like @felixdivo approves a change it shouldn't need me.

@hardbyte hardbyte removed their request for review December 12, 2021 19:54
@mergify mergify bot requested a review from hardbyte December 12, 2021 19:54
@felixdivo
Copy link
Collaborator

@felixn I just resolved the conflicts. If you address above comment, we can merge this contribution and include it in the next major release that will be coming out soon.

@felixn
Copy link
Author

felixn commented Dec 16, 2021

@hardbyte - thanks for the feedback! No worries, I completely understand. The time and motivation necessary for spare time open source projects is not always the same - at least, that's how it is for me.

@felixdivo - thanks for resolving the conflicts, and for proposing to include it in the next release. I made the suggested changes.
After that, I can spend some time on porting/including the additional functionality from @vincent-mailhol's PR.

@felixdivo felixdivo added this to the 4.0.0 Release milestone Dec 16, 2021
Copy link
Collaborator

@felixdivo felixdivo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just gonna change this small thing :)

@felixdivo felixdivo merged commit af0d832 into hardbyte:develop Dec 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants