pschichtel / JavaCAN

A simple JNI wrapper for the socketcan API provided by the Linux kernel. As it is wrapping a Linux Kernel API, it is intended for use on Linux only.
https://schich.tel
MIT License
49 stars 21 forks source link

Library is directly printing to stdout instead of using a framework like slf4j #25

Closed psobiech closed 3 years ago

psobiech commented 3 years ago

Hi, first of all great project, I was able to read data from my ECU using UDS really fast (after I realized I needed to enable TX padding..).

But my project is using slf4j, so having a library that is printing directly to stdout is not that great, since the users have no control over the logging.

I forked the project, added slf4j support and replaced all instances of using print*() with logger statements. I also configured the logging levels so the tests are not that verbose by default, this way any errors/warnings should be easier to spot.

The code is here: https://github.com/psobiech/JavaCAN/commit/731806e1e746cc36f59992ea770ce18af3bc1244

I do not know if you want this merged to mainline as there are dependent projects that would need to add at least slf4j-simple/slf4j-nop to work, so I am creating this discussion here.

PS: I also enabled compilation on newer java versions (I am using 11), since with source/target 1.8 everything seems to work just fine.

pschichtel commented 3 years ago

after I realized I needed to enable TX padding..

been there ... :D

I do not know if you want this merged to mainline as there are dependent projects that would need to add at least slf4j-simple/slf4j-nop to work, so I am creating this discussion here.

I'm definitely interested in this change. I'm actually kinda surprised that I never this myself. I'm usually the one complaining about libraries not using slf4j and here I am not doing it. Please open a PR, so we can get this in ASAP.

I also enabled compilation on newer java versions (I am using 11), since with source/target 1.8 everything seems to work just fine.

The library does have users that are constraint to 1.8, this is my way of forcing me and others to build and test against 1.8, so I'd prefer to keep the project as-is in that regard. Or do you have a compelling reason to change anything there?

psobiech commented 3 years ago

I mean it has source and target still 1.8, so the .jar should have no problems on Java 8 at least in theory, but it does not lock the JVM you are compiling on, as I do not have 8 installed even.

I checked some more sources and adding additional parameter <release>8</release> to the maven-compile-plugin should ensure that this is the case (https://www.baeldung.com/maven-java-version#java9)

psobiech commented 3 years ago

OK, I just checked and it's not possible sadly. The compiler plugin tries to pass the argument even for java 8, which results in an error.

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.0:compile (default-compile) on project javacan-core: Fatal error compiling: invalid flag: --release

I reverted the change and made MR only with slf4j ;-)

pschichtel commented 3 years ago

thanks for this! do you need this backported to a stable release or is it fine in master? I'm not sure if the current state in master is stable enough for a 3.0 release. I intended to wait until at least an early iteration of #20 is merged.

psobiech commented 3 years ago

I am using master right now, so no need to backport for me, just wanted to share my local changes upstream ;-)

Thanks!