TheBizzle
commented
12 years ago Alright, everything seems to be operating correctly. However, I apologize but I'm going to have to keep dragging this process out more and more for you; I'm declining this pull request and requesting that you to make some changes before submitting a new pull request–but don't worry; you're almost there!
Things That Need Addressing
Code Style
- Please use spaces in place of tabs
- Distributing code with tabs in it makes for viewing problems when other people try to read your code (myself included). I'm sure that, to you it was indented fine, but, to me, it appears to be indented much too far to the left. This is the fateful scourge of tab usage.
- Our coding standards ask that you use two spaces for indents (rather than the normal Java convention of using four spaces). It's not that you've violated that rule or anything (since you've been using tabs), but it's just an FYI.
- Is your editor set to convert tabs to spaces? (It's pretty much a standard in the world of programming that your editor should be set to use spaces.)
- Your code's spacing between operators is inconsistent with our existing codebase's spacing style
debugText+="RH1_"+Integer.toHexString((int)b&0xFF)+" ";should be respaced todebugText += "RH1_" + Integer.toHexString((int) b & 0xFF) + " ";- I'd like to see spaces added to the insides of curly braces in statements like
int[] right = {Syntax.NumberType()};, but, honestly... I won't refuse a pull request that doesn't do that.
- Unnecessary casts
- Lines 301, 304, and 322 contain unnecessary
(int)casts; these should be removed
- Lines 301, 304, and 322 contain unnecessary
Debugging Functionality
- You've added
debugTextand thesend,debug, andclearprimitives to the extension for what appears to be debugging purposes. Are those things really going to be desirable for the end user of the GoGo Sense to have on hand? (That's not a rhetorical question; I legitimately don't know and leave that up to you to decide.) If the GoGo Sense is working properly, and you don't believe that the end user will have reasonable use for the debugging commands, I ask that you remove the debugging primitives and thedebugTextvariable from the extension.
API Design
- If you keep the debugging stuff, there's still a matter of API design that needs addressing.
send,debug, andclearare very unclear in what their purposes are. What am I sending? What am I clearing? What exactly am I debugging? Perhaps it'll turn out that I'm just beating on a straw person with those three, but, regardless,msensoris also a bit confusing to me. Is there a way that it can be named such that someone can look at it and understand what it does just by its name? Also, can you document in the code how exactly it differs fromsensor? You say "A sensor on the GoGo Sense independent board", but I don't yet understand how that differs from the normalsensorprimitive. Further elaboration on that would be appreciated.
--Jason
MinasTyuru
The GoGoSense project is a project to expand the capabilities of the GoGo Board. This commit adds a command to NetLogo called "msensor." This requests an extended sensor value from the GoGo Board or independent GoGoSense board (which can connect to NetLogo directly, or connect to the GoGo Board).
The sensor request is sent in the form [out header] 0b11100000 [sensor number high byte] [low byte]. 0b11100000 does not collide with any of the commands listed in the GoGo Board serial protocol. The connected board responds with [in header] [sensor value high byte] [low byte], in the same way as it would reply to a normal sensor request.
This commit does not yet support burst mode for the extended sensor bank.
This commit also contains debug code. Each instance of GoGoController has a public String, debugText. Various methods append output to this string: specifically, when a method is waiting for input from the GoGo Board, it writes "[description of expected input]_[received input as hex string] ." For example, when waiting for the first reply header, "RH1_0xFF " could be appended to the string. Users can access the string with the command gogo:debug, and clear it with gogo:clear.