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

Clean CAN master's DeviceDriver methods #74

Closed
jgvictores opened this issue Sep 19, 2016 · 17 comments · Fixed by #229
Closed

Clean CAN master's DeviceDriver methods #74

jgvictores opened this issue Sep 19, 2016 · 17 comments · Fixed by #229

Comments

@jgvictores
Copy link
Member

jgvictores commented Sep 19, 2016

CanBusControlboard open() and close() functions contain class-specific functionalities that can and should be moved to the open() and close() functions of the corresponding classes "TechnosoftIpos", "CuiAbsolute", etc.

@PeterBowman
Copy link
Member

We spoke recently about moving CANopen-related method to a new interface (perhaps ICanOpen?), namely start, readyToSwitchOn, switchOn and enable (now located in ICanBusSharer.h), which would essentially end up leaving the most relevant method in the ICanBusSharer interface (and not much more): interpretMessage. Among what will be left, setCanBusPtr could be dropped in favour of passing an ICanBusHico pointer through YARP properties prior to each device initial configuration. We can see an example in the openrave-yarp-plugins tree: setter, getter. Furthermore, we would like to explore the idea of using multiple threads, one per device's open and close, in order to speed up things and avoid staging time-consuming CAN commands across several for-loops with delays (ref).

@jgvictores
Copy link
Member Author

  1. ICanOpen. Perfect name. I think those methods are good. Some more inspiration at 1, but before adding more methods it would be best to contrast at the official source whether if these are actual CAN Open commands (downside is accessing documentation is not easy).

  2. Replacing setCanBusPtr as described. Great.

  3. More than speeding up things through multiple threads, I'd start by replacing empirically set hard-coded delays by checks that assure the stages have been reached.

@PeterBowman
Copy link
Member

Among what will be left, setCanBusPtr could be dropped in favour of passing an ICanBusHico pointer through YARP properties prior to each device initial configuration.

Confirming that this was not meant to last per #158 (comment):

I like the idea of passing the pointer via blob. The additional setter methods were just hacks before I had experimented doing so on the openrave-yarp-plugins repo. :-)

See how to implement this with proper casts in #158 (comment).

@PeterBowman
Copy link
Member

PeterBowman commented Oct 17, 2018

We spoke recently about moving CANopen-related method to a new interface (perhaps ICanOpen?)

See https://github.com/canpie/CANpie (credits to @jgvictores for finding this).

Edit: beware of the Apache license! (https://opensource.stackexchange.com/a/5756)

@PeterBowman
Copy link
Member

PeterBowman commented Oct 17, 2018

I don't think we'd benefit from that library anyway, at least not in the scope of this issue:

It is not the intention of CANpie to incorporate higher layer functionality (e.g. CANopen, J1939).

We encapsulate CAN frame functionality within every CAN bus device (CanBusHico, CanBusPeak, future CanBusSocket and CanBusEsd, even CanBusFake with fake frames). There is no need to expose a common API in that respect as our intermediate layer is already the YARP ICanBus and related interfaces.

@jgvictores
Copy link
Member Author

Blocks #171.

@PeterBowman
Copy link
Member

Among what will be left, setCanBusPtr could be dropped in favour of passing an ICanBusHico pointer through YARP properties prior to each device initial configuration.

I'd rather abandon this approach, let's restrict CAN comms to the CanBusControlboard device (if possible): #209 (comment).

@PeterBowman
Copy link
Member

Recently, the ICanBusSharer::initialize method was added to configure drives via CAN SDO protocol prior to NMT initialization: 297b070.

@PeterBowman PeterBowman changed the title [CanBusControlboard] clean open() and close() Clean CAN master's implementation of DeviceDriver methods Aug 6, 2019
@PeterBowman PeterBowman changed the title Clean CAN master's implementation of DeviceDriver methods Clean CAN master's DeviceDriver methods Aug 6, 2019
@PeterBowman
Copy link
Member

Next step: re-design the ICanBusSharer interface (develop, current WIP).

@PeterBowman
Copy link
Member

Workflow:

  • Device instantiation: CanBusControlboard calls DeviceDriver::open on each raw subdevice.
  • Pass CAN sender handle (currently CanSenderDelegate, name is subject to change) so that subdevices can actually write stuff.
  • Initialization phase: this corresponds to the NMT pre-operational state for CANopen-enabled subdevices. Here, SDOs are used in order to perform initial configuration, including PDO mappings and communications. No PDOs allowed.
  • Issue NMT start command (CANopen-based): transition to operational state, PDOs allowed.
  • Commands related to CiA 402 state machine: shutdown, switch on, enable operation and disable voltage. I'm deferring the deletion or refactorization of these commands to issue Observe and perform state transitions with YARP control modes #215.
  • Interpret message: handles incoming CAN data.

So far, no changes?

@PeterBowman
Copy link
Member

PeterBowman commented Sep 10, 2019

I think the following comment from #211 (comment) belongs to here:

I'd fancy a .ini file structure that doesn't enforce properties for nodes which are never going to use them, e.g. maxVels in CuiAbsolute. Also, at some point in the future we might develop a motor drive node (analogous to TechnosoftIpos) which expects a different set of properties. Therefore, a plausible solution is to segregate those properties per CAN node type and place them in separate groups within the .ini file, e.g. [TechnosoftIpos_2], [CuiAbsolute_102] etc.. The CanBusControlboard wrapper should not need to parse and pass them along to wrapped subdevices, just take care of loading the .ini group on a per-subdevice initialization basis.

@PeterBowman
Copy link
Member

CuiAbsolute stuff cleaned at a0ad034, solved #207 by the way.

@PeterBowman
Copy link
Member

CiA state machine transitions (e.g. switch on, enable, etc.) with hardcoded timeouts refactored at 972f875.

@PeterBowman PeterBowman mentioned this issue Sep 13, 2019
26 tasks
@PeterBowman
Copy link
Member

PeterBowman commented Sep 13, 2019

Mode change shall be performed by the launcher application: 00916c9. CanBusControlboard's open procedure will be strictly focused on drive/node initialization, further actions are to be performed by external actors.

@PeterBowman
Copy link
Member

Homing refactored at e1995b6, see also #215 (comment).

@PeterBowman
Copy link
Member

PeterBowman commented Sep 25, 2019

Mostly ready at ad741da, see develop for comparison. The .ini file cleanup will be discussed in #237.

@PeterBowman
Copy link
Member

The workflow has been slightly altered to allow deferred CAN node initialization, see #242. Now, CanBusControlboard::open is supposed to create wrapped subdevices (CAN nodes and buses) and start threads. If possible, CAN nodes are started at this point, too, but this is not required for the application to keep running. On power-on, a boot-up signal is issued and caught by the monitor thread to initiate the CAN configuration sequence.

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

Successfully merging a pull request may close this issue.

2 participants