ISSUE-538: Added user defined CAN speed selection, by setting SJW, BR…

[ttcontrol-brunnthaler]

…P, TSEG2, TSEG1 with the bitrate argument

Closes #538.

[ttcontrol-brunnthaler]

#538

[codecov]

Codecov Report

Merging #539 into develop will decrease coverage by 0.03%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop     #539      +/-   ##
===========================================
- Coverage       64%   63.96%   -0.04%     
===========================================
  Files           63       63              
  Lines         5700     5703       +3     
===========================================
  Hits          3648     3648              
- Misses        2052     2055       +3

[felixdivo]

But thank you for the contribution! Where did you find the information about the custom bitrate? We might just link it in the parameter documentation of bitrate.

[ttcontrol-brunnthaler]

Hello felixdivo,
Sorry for answering this late!
There is this website: http://www.bittiming.can-wiki.info/ where you can find under the BOSCH 16 bit Bit Timing Register BTR how the custom CAN is built and where the bits are mapped too.
If you download the https://www.peak-system.com/Software-APIs.305.0.html PCANView you can enter the speed you want and get the Bus Timing Register as parameter back
Best regards

[felixdivo]

Okay, nice to see that Wiki! It would probably be nice to link it somewhere.

[ttcontrol-brunnthaler]

@felixdivo sorry I overwrote some of the changes you made because the main develop branch was extended to CAN-FD could you redo the merge you did before.
Best regards

[felixdivo]

No sorry, only you can commit to your own branch. I think you should undo the rebase and apply b96a54f on top of a rebase from the GUI in GitLab or a new clean rebase done from CLI. Something seemed to have gone wrong.

[felixdivo]

And also by now I forgot what I changed. ^^

[bhass1]

The proposed change doesn't seem to actually work. TPCANHandle(bitrate) isn't the proper interface.

It should be TPCANBaudrate(BTR0BTR1), and BTR0BTR1 needs to be calculated with an external tool which makes the current usage difficult for an ordinary user passing in arguments from the command line (i.e. simple 500000 does not translate directly to the proper BTR0BTR1 value for 500KBaud).

[bhass1]

It isn't clear how this actually works. Where are the SJW, BRP, TSEG1, and TSEG2 set? Does TPCANHandle(bitrate) handle arbitrary bitrates?

I don't see this interface in the Python PCANBasic API docs: https://www.peak-system.com/produktcd/Develop/PC%20interfaces/Windows/PCAN-Basic%20API/Include/PCANBasic.py

[ttcontrol-brunnthaler]

Its just a type definition:
TPCANHandle = c_ushort # Represents a PCAN hardware channel handle
But you are right I should use the :
TPCANBaudrate = c_ushort # Represents a PCAN Baud rate register value

"(i.e. simple 500000 does not translate directly to the proper BTR0BTR1 value for 500KBaud)"
In this case it does because we look the default values (most used) up in a dictionary
line 55: pcan_bitrate_objs
In the docstring I describe it quite well I think:
This argument can also be used for a user defined CAN-speed (e.g.: 0x0012 for 1.33 MHz)
For details on how to write a user speed register (SJW, TSEG 1/2, BRP) see:
http://www.bittiming.can-wiki.info (NXP SJA1000)
Please tell me what I could add for better understandability

[bhass1]

The problem I see is that bitrate is an integer representation of the bitrate (e.g. 500000). If user_bit is set, then line 97 becomes pcan_bitrate = TPCANHandle(500000) but in the dictionary, for 500K bitrate, pcan_bitrate = TPCANBaudrate(0x1C) #or TPCANBaudrate(28). They both can't be right.

The same issue comes up for the written example in the docstring. I'd expect bitrate to be 1330000 for 1.33M bitrate.

So the definition of what bitrate represents changes if user_bit = True or user_bit = False, and I don't think it's a good idea to do that.

Perhaps it would be better to define a new configuration type or calculate the register value of BTR0BTR1 within this method based on bitrate.

[felixdivo]

Is this solved? Once it is, I'd merge this PR.

[bhass1]

Still discussing in Issue #538

[ttcontrol-brunnthaler]

@felixdivo
On 4th of april: @felixdivo Merge branch 'develop' into develop d8f3495
Could you redo this merge please?

[felixdivo]

I don't know how to do this, maybe @hardbyte has other rights? But maybe it's not possible since you applied changes on top of it.

[hardbyte]

@bhass1 are you happy to sign off on this? I'd like to hold off merging until I see your approving review.

I'm going to start getting develop ready for the 3.2 release so will target this feature for the next release - either 3.3 or 4.0.

[bhass1]

@bhass1 are you happy to sign off on this? I'd like to hold off merging until I see your approving review.

I don't like the user-interface - it muddles the meaning of bitrate. I am too busy now to commit the changes to add a separate configuration parameter, btr0btr1. When is 3.3 due out?

[felixdivo]

The next release will now be 4.0. It will be some weeks until we release it, since there are quite a few major changes, like dropping support for all Python versions below 3.6 and reworking how exceptions are used. Take your time.

[marcel-kanter]

+1 for the separation of bitrate and btr0 btr1.
These settings seem to be available for many can controllers.

[felixdivo]

See #615

[felixdivo]

#615 is now merged.

[karlding]

Closing since this is superseeded by #625. Please reopen if necessary.