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.

Sign up for GitHub

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

Jump to bottom

Make sure conversion is complete; fix ads1015 scaling. #174

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Make sure conversion is complete; fix ads1015 scaling. #174

wants to merge 4 commits into from

Conversation

fovea1959
Copy link

Fixing two ads1x15 issues:

  1. just waiting for an amount of time is not always sufficient. Changed code to wait for the conversion complete bit.

  2. range mapping for ads1015 was not correct.

@fovea1959
Copy link
Author

fovea1959 commented Jul 7, 2023 via email

@fovea1959
Copy link
Author

fovea1959 commented Jul 7, 2023 via email

EAGrahamJr
EAGrahamJr reviewed Jul 14, 2023
View reviewed changes
Copy link
Contributor

@EAGrahamJr EAGrahamJr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the my main issue with the logging is that it does add overhead (each function is an anonymous class which then has a concrete implementation created, which may or may not ever be used).

It also doesn't look like any of the other classes/logging, so you've basically introduced a different style than everything around it.

But that's all up to Matt... 😀

@mattjlewis
Copy link
Owner

Taking a look - the amTracing instance variable is a little unusual.

@EAGrahamJr
Copy link
Contributor

Taking a look - the amTracing instance variable is a little unusual.

That's kind of common practice to ensure that specific instructions are not run when a method is invoked. In this case, I believe the intent was to avoid any overhead when logging calls are made - e.g. when the trace method is called with method invocations in the parameter list, the parameters are evaluated prior to method invocation. So

Logger.trace("msb: 0x{}, lsb: 0x{}", Integer.toHexString(config_msb & 0xff),
				Integer.toHexString(config_lsb & 0xff));

always calls both toHexString methods, whether tracing is enabled or not. If you put the if (amTracing) in front of that, nothing gets evaluated except the if.

On the other hand, the PR changes also tries to get around calling "un-necessary" functions through the use of functional interfaces.

Logger.trace("setConfig: 0x{} 0x{}", () -> Integer.toHexString(config_msb & 0xff),
			() -> Integer.toHexString(config_lsb & 0xff));

But there's even more overhead because

  1. creates an anonymous class for each defined block
  2. creates an instance of said class (so there's 2 classes and 2 instances above)
  3. invokes the function each time trace is called (because parameters are evaluated first)
  4. which calls the method we were trying to avoid calling in the first place

Item 3 is the actual "gotcha" that a lot of people just really don't know about.

@fovea1959
Copy link
Author

But there's even more overhead because

1. creates an anonymous class for _each_ defined block

yep, you are correct. Not sure this is a runtime issue.

2. creates an instance of said class (so there's 2 classes and 2 instances above)

yep, you are correct. But the class will only be loaded once from the classpath, which is expensive compared to running the constructor.

If I had instantiated the Suppliers ONCE and then passed the instantiated objects to the logging calls, that would avoid the "multiple instantiations of the (already loaded) classes" issue that you have called out here. The calls I guarded/avoided with the lambdas are probably not expensive enough to be worth it. So, I removed the lambdas.

3. invokes the function each time `trace` is called (because parameters are evaluated **first**)

just so we all know, however, I'm pretty sure I'm on solid ground on this one. tinylog does not invoke the lazy parameter Supplier<> functions unless the logging is actually taking place. I confirmed this in the source code, and via experimentation (test case pasted below). Deferring the execution of the Supplier<> functions until they are needed is precisely why the tinylog (and log4j2) designers put those signatures in there, but given points 1 and 2 above, using the lambdas is probably appropriate for more expensive operations than the ones we are talking about here.

TinylogTest.java:

public class TinylogTest {
    public static void main(String[] args) {
        new TinylogTest().go();
    }

    boolean expensiveOperationExecuted = false;

    void go() {
        expensiveOperationExecuted = false;
        org.tinylog.Logger.info("Info: {}", () -> expensiveOperation());
        System.out.println ("expensive operation was invoked by .info() = " + expensiveOperationExecuted);

        expensiveOperationExecuted = false;
        org.tinylog.Logger.debug("Debug: {}", () -> expensiveOperation());
        System.out.println ("expensive operation was invoked by .debug() = " + expensiveOperationExecuted);
    }

    String expensiveOperation() {
        expensiveOperationExecuted = true;
        return "Hi, Bob!";
    }
}

log4j2.xml:

<?xml version="1.0" encoding="UTF-8" ?>
<Configuration status="WARN">
    <Appenders>
        <Console name="console" target="SYSTEM_OUT" follow="false">
        </Console>
    </Appenders>
    <Loggers>
        <Root level="info">
            <AppenderRef ref="console"/>
        </Root>
    </Loggers>
</Configuration>

build.gradle:

plugins {
    id 'java'
}

group 'org.example'
version '1.0-SNAPSHOT'

repositories {
    mavenCentral()
}

dependencies {
    implementation 'org.tinylog:tinylog-jboss:2.5.0'
    implementation 'org.apache.logging.log4j:log4j-core:2.20.0'

    testImplementation 'org.junit.jupiter:junit-jupiter-api:5.7.2'
    testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.7.2'
}

test {
    useJUnitPlatform()
}

Program output:

> Task :TinylogTest.main()
Info: Hi, Bob!
expensive operation was invoked by .info() = true
expensive operation was invoked by .debug() = false

@EAGrahamJr
Copy link
Contributor

just so we all know, however, I'm pretty sure I'm on solid ground on this one. tinylog does not invoke the lazy parameter Supplier<> functions unless the logging is actually taking place.

TIL - I didn't know TinyLog took a list of parameter suppliers - yes, you are correct. (I was expecting Object... rather than Supplier<Object>... - guess I should have checked 😀 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EAGrahamJr EAGrahamJr EAGrahamJr left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fovea1959 @mattjlewis @EAGrahamJr