search

SoundMetrics / aris-integration-sdk

SDK for building custom controllers for the ARIS sonar.
MIT License
5 stars 2 forks source link

Initial spike for Simplified Protocol. #140

Closed curtnichols closed 4 years ago

curtnichols commented 4 years ago

PR for automated build.

curtnichols commented 4 years ago

@billhanot Could you give the readme.md (link) a read-through and comment for clarity of purpose and concepts? I'm mostly looking for 9th-grade prose, mid-senior-level understanding of programming.

Feel free to comment here on this PR, leaving review comments in markdown files can be awkward.

billhanot commented 4 years ago

In my comments below, the numbers refer to the line numbers in the diff text when I clicked on the link. I reviewed the draft README.md via the View file control and then matched my observations with the line numbers.

46 Perhaps expand “RTC” to “sonar RTC” or “RTC (sonar clock)? I’m not sure if RTC is universally understood outside of English. 94 “when settings a range” should be “when setting a range” Perhaps change “altered somewhat” to “constrained” to match later discussion? 130 “cookie apperas” should be “cookie appears” 137 Change “the effect of electrical noise” to “the presence of electrical or acoustic noise” 222 "header_ size” should be “headersize" 223 “frame size” should be “framesize" 224 “frame index” should be “frameindex” 225 “part number” should be “partnumber” 226 Probably just me, but I had to look up “octet” to see it’s equivalent to an 8-bit byte. Perhaps it could read “octets (8-bit bytes)”? 227 “parkt header_size” should be “part_header_size” 229 Perhaps explicitly state that “10 samples per beam” is for illustration only and the minimum legal value is 100?

Must the command arguments be in the order listed (state yes or no explicitly)? Does the integrator need to (or should?) keep track of the expected cookie values if matching commands with returned frame headers? Is “payload” a universal nomenclature? If not, perhaps add a definition?

Other than that, everything is clear and concise from my point of view.

[Removed email signature and unnecessary email bits. -Curt]

curtnichols commented 4 years ago

229 Perhaps explicitly state that “10 samples per beam” is for illustration only and the minimum legal value is 100?

Illustrations are for illustrative purposes. ARIScope sets the floor to 200 samples, so that's what is documented here. Fewer seems not so useful. The advice we're presently giving is to leave samples at the default of 1000.

Also, yes, octet has been around a long time. It's still the correct term for discussing network protocols. Unlike MB and GB.