Skip to content

Added support for bus statistics in the kvaser interface.#477

Merged
hardbyte merged 5 commits intohardbyte:developfrom
sIuv:develop
Dec 26, 2018
Merged

Added support for bus statistics in the kvaser interface.#477
hardbyte merged 5 commits intohardbyte:developfrom
sIuv:develop

Conversation

@sIuv
Copy link
Copy Markdown
Contributor

@sIuv sIuv commented Dec 3, 2018

No description provided.

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 3, 2018

Codecov Report

Merging #477 into develop will increase coverage by <.01%.
The diff coverage is 64.28%.

@@             Coverage Diff             @@
##           develop     #477      +/-   ##
===========================================
+ Coverage    63.66%   63.66%   +<.01%     
===========================================
  Files           55       56       +1     
  Lines         4687     4715      +28     
===========================================
+ Hits          2984     3002      +18     
- Misses        1703     1713      +10

Copy link
Copy Markdown
Owner

@hardbyte hardbyte left a comment

Choose a reason for hiding this comment

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

Hi @sIuv - appreciate you taking the time to open this PR (GitHub tells me it's your first ever - if so welcome to the world of open source! 👋 )

If at all possible I like to avoid having backend specific functionality - just hanging a getstats on the Bus that only works for Kvaser isn't ideal. However to address this properly we will have to design a cross backend implementation... so for now I think your changes are okay to include.

One thing I'd like to be added before we can merge this change is documentation - Kvaser users will read this page and should get a good idea of what the custom method will do and return.

Contains Python equivalents of the structures in CANLIB's canstat.h,
with some supporting functionality specific to Python.

Copyright (C) 2010 Dynamic Controls
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Unless I'm missing something this isn't required

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the fast response. Noticed that the reference to canstat.h was not correct either.

@sIuv
Copy link
Copy Markdown
Contributor Author

sIuv commented Dec 11, 2018

For documentation, would it be appropriate to remove get_stats form the bus section and add a new section.
E.g. named "Custom methods" and include it there instead.

Bus


.. autoclass <...>.KvaserBus
	:members:
	:undoc-members: get_stats

Custom methods
~~~~~~~~~~~~~~~~~~

.. automethod <...>.KvaserBus.getstats

@hardbyte
Copy link
Copy Markdown
Owner

Yeah documenting custom methods like that sounds like a good approach.

@sIuv
Copy link
Copy Markdown
Contributor Author

sIuv commented Dec 26, 2018

Moved get stats to a separate documentation section like the suggestion above.

Copy link
Copy Markdown
Owner

@hardbyte hardbyte left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes. Looks all good to me.

@hardbyte hardbyte merged commit 69db2ec into hardbyte:develop Dec 26, 2018
@hardbyte hardbyte added this to the 3.1 Release milestone Jan 7, 2019
@nitishn23
Copy link
Copy Markdown

nitishn23 commented Apr 14, 2023

Hello Can you please suggest is this good way to get bus statistics for the Kvaser interface, i am using "can.interface.Bus.state()"this method to get bus status nfortunately it always returns as Active when the actual state is Passive. I checked the "can.interface.get_stats()" it does return with high bus load when the connection is lost with some error frames.

image

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

Successfully merging this pull request may close these issues.

3 participants