Basic support in java: EasyGoPiGo3 motor and LED control #250
Conversation
Fix firmware version in NodeJS
|
@RobertLucian This code is going into Java_New for now, so we will end up with Java (already existing code) and Java_New (with a more complete support). Both of these took different approaches. Java_New creates a package, and the old one doesn't, for example. |
There was a problem hiding this comment.
Regarding the example program: it doesn't work. Running it makes the GoPiGo3 go forward for a couple of seconds and then it starts orbiting towards right without ever stopping.
Next, there is a lot of code coming from Ken Fogel. His signature is in most comments/documentation. I think that only clutters the documentation overall - I'd only mention him at the top of a module. Him and jabrena too.
Regarding the documentation, it is still mostly missing:
- There are methods that don't have any documentation at all.
- Some of them that do have are not descriptive enough: for instance it is not said what each parameter does, what it is the range of each one and what the returned value represents. Since Java is a strongly typed language, I'd also expect to see more emphasis on describing the parameters.
- Describe when a exception occurs for a given method.
Next, still regarding the documentation, but referring to the README, I don't see one at all. We need one so that the user knows what has to be done. Installation instructions, description of the documentation (maybe even breaking down the module(s) in a succint manner), usage tips/instructions, tips on how to set up the CLASSPATH and so on. You can probably take other READMEs from this repo as a guide.
Moving forward, the old project was built around Gradle. Why discard Gradle and move to a more complex, possibly more verbose solution? Apart from building this as a package, which is a good move, the old project was already on the right track. If not Gradle, then maybe Maven or the many other alternatives there are today. If we don't move on to using an dependency manager, then at least let's have some instructions on how to install the dependencies: the pi4j and json-simple packages. They both need their own directories and the CLASSPATH variable needs to be exported.
Why should there be 2 versions of the Java library? I think that is very confusing to any user - I know it is for me, so it must be for others as well. As long as the library is for the GoPiGo3, then there should only be one directory called Java just like there is one for Python or for C++ or whatever else. Maybe I'm not following it, but could you explain why there should be 2 of them?
And finally, regarding the .gitignore file, I'd actually prefer having only one on this repo. This whole repo ain't a big project, so we can deal with only one. Multiple .gitignores bring in too much complexity that's not needed. In the root .gitignore, I'd ignore all .class files regardless.
Also as a final note, I didn't add comments everywhere in the source code. I just gave a couple of examples. Take the comments as a guide for all the rest.
Everything else looks fine to me.
| * @author Ken Fogel | ||
| * @param args | ||
| */ | ||
| public void spiArrayBuilder(byte... args) { |
There was a problem hiding this comment.
I think this should be private.
| } | ||
| } | ||
|
|
||
| public void spi_array_builder(byte... args) { |
There was a problem hiding this comment.
Let's make this one private too.
| return (int)(encoder / MOTOR_TICKS_PER_DEGREE); | ||
| } | ||
|
|
||
| public void offset_motor_encoder(byte motor, int offset) throws IOException { |
| spi.write(spi_array_out); | ||
| } | ||
|
|
||
| public void reset_motor_encoder(byte motor) throws IOException { |
| * @return | ||
| * @throws IOException | ||
| */ | ||
| public int get_grove_analog(byte pin) throws IOException { |
There was a problem hiding this comment.
What is the range of the pin parameter? This is a recurring thing with other parameters of other methods as well.
| spi_array_in = spi.write(spi_array_out); | ||
| } | ||
|
|
||
| public void set_grove_mode(byte pin, byte mode) throws IOException { |
There was a problem hiding this comment.
Missing documentation.
| * TODO: bugfix this function too | ||
| * @return | ||
| */ | ||
| public double get_voltage_battery() { |
There was a problem hiding this comment.
Does this method work or not? I see there's a bug report up here listed in the documentation.
| * @author Ken Fogel | ||
| * @return | ||
| */ | ||
| public String get_version_firmware() { |
There was a problem hiding this comment.
I see Ken Fogel appearing everywhere.
| * @throws IOException | ||
| * @throws InterruptedException | ||
| */ | ||
| public void reset_encoders(boolean blocking) throws IOException, InterruptedException { |
There was a problem hiding this comment.
The double asterisks are not needed here. It's valid for the Python version, but not for the Java one.
| * @return | ||
| * @throws IOException | ||
| */ | ||
| public boolean target_reached(double left_target_degrees, double right_target_degrees) throws IOException { |
There was a problem hiding this comment.
Since this is Java and not Python, the returned values are going to be true and false and not True and False - this is regarding the documentation.
No description provided.