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

Access to plain channel which do not parse frames #22

Closed
splatch opened this issue Aug 28, 2020 · 10 comments
Closed

Access to plain channel which do not parse frames #22

splatch opened this issue Aug 28, 2020 · 10 comments

Comments

@splatch
Copy link
Contributor

splatch commented Aug 28, 2020

This might be similar to #20 especially that I began using your code together with netty.
I've successfully integrated your library with Apache PLC4X project under plc4x-transport-socketcan in a way which is working, but still seems semi optimal. Within PLC4X we do usually generate code related to handling of exchanged payloads (frames, structs and such) so we don't need a CANFrame as we have our own representation for that. We're perfectly fine with raw stream of bytes.

For reference you can take a look on:
https://github.com/apache/plc4x/blob/537f4011ef93e09a04446cc520ba16b4a9258d9f/plc4j/transports/socketcan/src/main/java/org/apache/plc4x/java/transport/socketcan/netty/SocketCANChannel.java#L116L121
https://github.com/apache/plc4x/blob/537f4011ef93e09a04446cc520ba16b4a9258d9f/plc4j/transports/socketcan/src/main/java/org/apache/plc4x/java/transport/socketcan/netty/SocketCANChannel.java#L247

These are places where we explicitly touch JavaCAN APIs in which as you can see we just wrap or unwrap payload.

@pschichtel
Copy link
Owner

Correct me if I'm wrong: You are asking for an even lower-level API that skips the CanFrame wrapper entirely.

@splatch
Copy link
Contributor Author

splatch commented Aug 31, 2020

Yes, I am asking if you would consider dropping wrapping in one or another implementation of channels entirely. Since plc4x has its own ways for mapping payload there is no point to do it twice. After reading your code and doing complete read-write part I learned that socketcan returns fixed length chunks of data (16 or 72 bytes). I already arranged handling of data padding in plc4x so we're good to go one level down.

Given that your library is compatible with Apache license, has quite nice API it enables us to use socketcan as one of transport options for CAN frames. I didn't work with BDM/iso-tp part of APIs yet. So far we're only on CAN_RAW.

@splatch
Copy link
Contributor Author

splatch commented Aug 31, 2020

Also, if you will take a look on where we ended up with socketcan-transport it is mapping frame back to byte buffer and then pushing it via old IO support provided by netty to get it parsed by PLC4X payload mapping.

@pschichtel
Copy link
Owner

If you look here, here and here, you'll see that

ByteBuffer buf = ....;
CanFrame frame = RawCanChannel.read(buf);
assert buf == frame.getBuffer()

holds.

The CanFrame.create*(ByteBuffer) methods, especially createUnsafe just do very basic sanity checking on the buffer. All getters directly poke into the buffer, no parsing is done on the data, so this is already a very thin wrapper.

But still, I can easily just expose the existing readSocket and writeSocket methods, that take a ByteBuffer. That would directly call into JNI and never touch the buffer contents otherwise.

@splatch
Copy link
Contributor Author

splatch commented Aug 31, 2020

But still, I can easily just expose the existing readSocket and writeSocket methods, that take a ByteBuffer. That would directly call into JNI and never touch the buffer contents otherwise.

That'd be awesome. I looked at these and tried to create PlainChannel in plc4x transport (since raw was already booked) but I've ran into visibility troubles of the method/classes. That's why I come to consult how this could be done. I believe it could play nicely with further work related to netty as plc4x uses netty pipeline to chain "handlers".

pschichtel added a commit that referenced this issue Aug 31, 2020
…ering

This is primarily to better support downstream libraries. See #22
@pschichtel
Copy link
Owner

pschichtel commented Aug 31, 2020

Would this work for you: 0edb265 ? If so I can backport that to 2.x

@splatch
Copy link
Contributor Author

splatch commented Aug 31, 2020

Yeah - its gonna definitelly work for plc4x. Thank you very much for exposing these!

pschichtel added a commit that referenced this issue Aug 31, 2020
…ering

This is primarily to better support downstream libraries. See #22

(cherry picked from commit 0edb265)
@pschichtel
Copy link
Owner

I just went ahead and released this as 2.3.0.

@pschichtel
Copy link
Owner

Do you think it would be possible to contribute the netty part of your PLC4X work back as part of #20 ?

@splatch
Copy link
Contributor Author

splatch commented Aug 31, 2020

I don't think there is a major issue with bringing it to your project, but I have to consult it with others what's their perception on that. The netty part we have pulls some project specific interfaces, but can be safely separated and run independently of PLC4X.

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

No branches or pull requests

2 participants