-
Notifications
You must be signed in to change notification settings - Fork 6
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
Revised component catalogue #60
Conversation
new run-configuration 'Rerun version on Pi'
major refactoring of SimpleLed (and LedButton)
refactoring of Buzzer
Components can deliver a mock now. Although this mixes test-specific code and productive code, overall it seems to be a better solution. The encapsulation of the components is better and there's no need for additional interfaces anymore.
LED now on SpiBus.BUS_1 (because UART is enabled now)
Sonatype Lift is retiringSonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too much for a full code review, but after a quick look, I only have small remarks. I love the improvements and additional comments as they make the code cleaner and easier to understand.
pom.xml
Outdated
@@ -43,7 +44,7 @@ | |||
<pi4j.version>2.3.0-SNAPSHOT</pi4j.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be <pi4j.version>2.3.0</pi4j.version>
@@ -1,8 +1,8 @@ | |||
package com.pi4j.catalog; | |||
|
|||
import com.pi4j.context.Context; | |||
import java.time.Duration; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a "beauty remark". When using auto save in IntelliJ IDEA with cleanup, imports should get sorted alphabetically. It's not critical at all, but will avoid code changes between commits, where imports only move place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do a 'optimize imports'
double aIn2 = adc.readValue(Ads1115.Channel.A2); | ||
double aIn3 = adc.readValue(Ads1115.Channel.A3); | ||
System.out.printf("Voltages: a0=%.3f V, a1=%.3f V, a2=%.3f V, a3=%.3f V%n", aIn0, aIn1, aIn2, aIn3); | ||
|
||
System.out.println("Single read done."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid the use of System.out but have all output via logger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the apps are using System.out and the components use logging.
Thought it might be easier to read.
@eitch What do you think? Use logging everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the logging can output the class name, it should be easy to distinguish the output, but nothing is "set in stone"
new Buzzer.Sound(PAUSE, 8) | ||
); | ||
|
||
private final List<Buzzer.Sound> imperialMarch = List.of( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need a nerdy example here? Star Wars? ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore above comment :-)
TestCases updated (still need additional review)
# Conflicts: # pom.xml # src/main/java/com/pi4j/catalog/Launcher.java # src/main/java/com/pi4j/catalog/applications/Ads1115_App.java
No description provided.