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

Periodic send api #80

Closed
hardbyte opened this issue Aug 31, 2016 · 10 comments
Closed

Periodic send api #80

hardbyte opened this issue Aug 31, 2016 · 10 comments
Assignees
Milestone

Comments

@hardbyte
Copy link
Owner

Originally reported by: Brian Thorne (Bitbucket: hardbyte, GitHub: hardbyte)


The interfaces in can/broadcastmanager.py should take python-can Bus instances not channels - this would be more backend agnostic.

It might be worth adding this periodic api directly to the bus itself too.


@hardbyte
Copy link
Owner Author

hardbyte commented Dec 28, 2016

So here is what I'm thinking, as always I'd appreciate any thoughts and feedback. Primarily I want the API to be as simple as possible for users, and also straightforward to implement for each backend.

I'm in favor of extending the Bus directly with a send_periodic method. This method could either take a CyclicSendTask argument or, a Message and period information. I'm not quite convinced either way yet so I'd like to discuss the pros and cons...

Using a CyclicTask object:

  • (-) More complexity for users. Most users probably just want the most basic of periodic sending to work really well and I suspect an api like bus.send_periodic(msg, period, repeats) might cater to most use cases.
  • (+) More flexibility for users. We could use the same Bus method to pass a few different types of tasks (e.g MultiRateCyclicSendTask or more complex instructions). We could have a library dispatcher and default pure python implementations and leave it up to each interface to override pieces.
  • (+) Might be more explicit because the user would have created a "Task" object which could later be used to stop and modify the task. As opposed to sending a periodic message and having to keep the returned PeriodicTask.
  • What else?

This public api Bus level method would probably return an object like PeriodicTask which could be used to modify or cancel it later. This has the potential for introducing threading issues so would have to be designed with something like queues or events to prevent them.

We can implement a concrete default implementation of this entire idea in the Bus's abstract base class (eg see christiansandberg:cyclic-fallback branch) and each interface would be encouraged to pass off to the hardware where it makes the most sense for them. IMO The tricky part is working out the api.

@christiansandberg
Copy link
Collaborator

It's a tricky question. I think it should be started using an existing Bus instance, since some interfaces require a handle and even if they only require a channel name then that could be read from the Bus instance. I guess usually you would like to both send and receive messages.

We could have both an API with a CyclicTask class where you pass a Bus instance as argument and a bus.send_periodic() method which could be a short-hand for creating that class.

I started the thread based fallback but I'm a bit unsure how to prevent calling the send() method simultaneously from different threads if it is used by both cyclic send tasks and for regular sending. Right now each interface would have to protect their send function with locks.

Socketcan

Uses channel name when configuring BCM. Bitrate is determined when setting up the link externally.

Kvaser

Has "object buffers" for sending periodically. Requires a handle to configure it.

NI-CAN

Has "CAN objects" for sending periodically. Can be configured with a channel name but probably requires the CAN-bus to be configured beforehand with correct bitrate etc.

Remote

Would benefit from re-using an existing bus connection instead of creating one connection for each task.

@hardbyte
Copy link
Owner Author

Ok starting to have a crack at the bus.send_periodic api in branch feature-periodic-send-api.

Another more minor threading concern crops up with dealing with task duration for implementations that don't have that as part of the hardware/kernel api. For example I put a threading.Timer call in socketcan which will require Python to stay alive. At this stage it is in the interface specific code, but could be moved to the base class.

Still feel that exploring the hardware apis more will help find the best common ground.

  • Should we include duration in the api - especially if no backends implement it.
  • Should a Bus keep track of all of the CyclicTask instances created by periodic_task?
  • Is CyclicTask.start useful? Would it be required/available for all implementations?
  • Is CyclicTask.modify_data avaliable for all implementations?
  • Should it be task.cancel or task.stop?

@christiansandberg
Copy link
Collaborator

christiansandberg commented Jan 16, 2017

I don't know what the need for a duration is but Kvaser would support it (canObjBufSetMsgCount) while NI-CAN does not.
Both supports modify_data. Kvaser also supports changing CAN-ID but not NI-CAN.
Kvaser supports starting and stopping apart from canceling the task but NI-CAN only supports canceling.

It would probably be good if the bus instance could stop all tasks on shutdown.

Here is the APIs for Kvaser and NI-CAN:

@pevsonic
Copy link

Hiya. I'm just dropping in to say hello as I've been working on some BCM changes locally which I suspect potentially cover some common ground.

I've been working on supporting more of the (kernel) broadcast manager API via python-can, in particular, adding RX content subscription and handling responses such as TX_STATUS and TX_EXPIRED, RX_EXPIRED etc. I've got a test version working but given the sensible ideas above, I think it makes sense to follow and extending the Bus directly to do this in the same way.

What I can't quite see is what the right way should be to extend Bus to handle the asynchronous responses and how subscribed rx content returned via the RX_CHANGED response should be handled in a sensible manner and how that would sit alongside (or replace?) to the existing Bus:recv() method?

Apologies if this should really be a new thread rather than extending this but I think it's probably part of the same discussion...!

@hardbyte
Copy link
Owner Author

Sorry I've gotten more than a little distracted with politics recently... if anyone does feel like taking this on I'll not take offense!

I've found duration useful - but usually not requiring high accuracy so I don't think it would be overly missed.

@pevsonic that is very interesting, I'd always glossed over the receive side of bcm as I hadn't quite understood the point! Only looking at it now do I see that it can notify you when the message content has changed. That actually does sound very useful! I think due to the ever expanding scope of v2.0 I'll ask that we mostly concentrate on the transmit side here and follow up in another issue regarding rx. - Please do stay involved though!

As another aside, I noticed another python can project can4python that offers bcm support - perhaps an opportunity there. I haven't reached out or looked closely at the code.

@christiansandberg
Copy link
Collaborator

I've added a thread based fallback to your branch, hope you don't mind. Feel free to change anything.

@christiansandberg
Copy link
Collaborator

Just wanted to add that apart from Kvaser and NI-CAN, IXXAT also has support for periodic transmits. Hopefully someone with access to the right hardware and time may implement some of these in the future... :)

@christiansandberg
Copy link
Collaborator

I think the API looks good as it is now. Should we close this issue?

@hardbyte
Copy link
Owner Author

Sounds good :-)

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

No branches or pull requests

3 participants