Exposed frameFD.flags (for bitrate switching) #106
Conversation
|
Anybody alive there? This modification is rather important. |
|
Hi and thanks for the pull request. But the change can't work. You are reading the field an object which doesn't have the field. You have to extend socketcan.json:363. Please also make sure the way you introduce it must be compatible with existing implementation and should cover error cases too. |
|
Hi @sebi2k1, thank you for your answer. Sebi, you write that it can't work, yet it DO WORK for me - tested. Please note that not all users of node-can use "DatabaseService" - some do call "sendFD(...)" directly. And to be fair with you, I don't feel to be familiar enough with DatabaseService to change the code there (yes, I could guess, but I cannot test) so better approach than ask to pull non-tested and possible buggy code is to hope that you are willing to fill in the missing things to complete it if needed. node-can is awesome and gives the Node.js the ability to handle CAN bus. It knows CAN-FD. Let's give it possibility to use it fully. Thanks, sorry for possible additional work for you and have a nice day @sebi2k1 Martin |
|
@wdim0 I think you are misunderstanding the answer of @sebi2k1. His point was not that hardcoding in flags on the native side does not work technically. His point was that this PR cannot be accepted since adding support requires implementing the passing of these flags from js to native. And that prior validation, error handling and testing is required. To the tone: This is an open source project and as far as I understand that, you are not entitled to allocate the time @sebi2k1 can spent. If you want it fast, you could get into it and flesh out the PR as required. If you cannot do that yourself, maybe you can ask @sebi2k1 to add a "help wanted" tag. I think this is a wonderful and straightforward task to get into open source contribution. My best regards. |
|
Hi @juleq, I'm sorry if what I wrote left an impression of a bad tone. I don't want to offend anybody and I don't want to demand anybody's time. I'm really grateful for node-can and for what all the contributors of node-can have achieved. I don't need it fast. I have what I need working already. I just wanted to share this IMHO important improvement with others - to give to the community, not just take. But yes, my modification is not visible to "DatabaseService". If the PR would be accepted, it will not break anything, it's backward compatible, but it exposes the new fd_flags only to users of native side (users like for example https://github.com/grodansparadis/node-red-contrib-socketcan is), not to the "DatabaseService" users. Maybe somebody sometime (not just @sebi2k1) will pick it up later and implement it for "DatabaseService" when needed. @sebi2k1 - I'm sorry if I offended you. Please accept my apology in that case. a) add a "help wanted" tag as @juleq suggests or Line 378 in 5f30eb6 If you don't see any point in possibility to be able to produce CAN-FD frames with bitrate switching, then please close/discard this PR. Thank you, all the good to you and have a nice day Martin |
There was a problem hiding this comment.
I am fine with the change if we limit it do the RawChannel for now. Proper integration of KCD (if supported) can be separated.
The only remark I have is that the flags should not be passed as a bitfield which requires the caller to know/understand socket can kernel headers files. I'd prefer to pass the individual flags as e.g. bools.
src/rawchannel.cc
Outdated
| @@ -375,13 +376,16 @@ class RawChannel : public Nan::ObjectWrap | |||
| CHECK_CONDITION(info[0]->IsObject(), "First argument must be an Object"); | |||
| CHECK_CONDITION(hw->IsValid(), "Invalid channel!"); | |||
| struct canfd_frame frameFD; | |||
| frameFD.flags = 0; // Flags not used in this function | |||
| //frameFD.flags = 0; // Flags not used in this function | |||
There was a problem hiding this comment.
Either remove the line or keep. No dead code please.
src/rawchannel.cc
Outdated
| @@ -64,7 +64,8 @@ using namespace v8; | |||
| #define rtr_symbol SYMBOL("rtr") | |||
| #define err_symbol SYMBOL("err") | |||
| #define data_symbol SYMBOL("data") | |||
| #define canfd_symbol SYMBOL("canfd") | |||
| #define canfd_symbol SYMBOL("canfd") | |||
| #define fdflags_symbol SYMBOL("fd_flags") | |||
There was a problem hiding this comment.
Please reconsider the approach. The basic idea of this API is to hide any kernel headers, so instead of pass a bit field of fdflags, I'd prefer to provide the individual flags. E.g. if someone wanna use bitrate switch you might call send(brs: True) instead of send(fdflags:3).
Hi,
I've made an important modification that enables users of node-can to do bitrate switching for CAN-FD frames.
Until now it was impossible (nobody complained yet?) - there was no way how to perform bitrate switching (set BRS bit in CAN-FD frame).
Bitrate switching is one of the main points of "FD" = "Flexible Data-rate".
frameFD.flags in SendFD method is now filled from new "fd_flags" symbol (feel free to rename if you come up with better name).
In the original code, the frameFD.flags was hardcoded to 0. But we need to be able to influence bit 2..0 in this flags field in order to fully control the transmission behavior.
Let's see how official "cansend" tool handles it and see the meaning of the flags:
This is a FD frame without bitrate switching (captured during my initial testing)
And this is FD frame with bitrate switching after the modification. fd_flags / flags is set to 0x01 in the "caller" code.
(same data as above)
Anyway, thanks for node-can!
Have a nice day
Martin