Add documentation for custom arduino commands #87

PeterJCLaw · 2018-01-31T16:08:12Z

This includes a summary of how to create the command on the Arduino as well as how to call it from Python and the errors which could be thrown.

See sourcebots/robot-api#55 and sourcebots/robotd#35 for the related implementations.

Note: there are some TODOs inline. The ones I think need fixing before we ship are:

understanding (and documenting) what happens to values which are sent as numbers (either float or integer)
what kind of Arduino is it?

This includes a summary of how to create the command on the Arduino as well as how to call it from Python and the errors which could be thrown.

I was going to use this to show how to do comments, but I think it's probably better that we don't do that for now.

RealOrangeOne · 2018-01-31T21:24:20Z

content/api/servo-assembly/arduino-commands.md

+running on the Raspberry Pi, you'll need to base it on SourceBots default
+firmware. You can get a copy of the source code from GitHub:
+
+```none


Can we use txt here? I'm not even sure none is a valid highlight

RealOrangeOne · 2018-01-31T21:25:02Z

content/api/servo-assembly/arduino-commands.md

+git clone https://github.com/sourcebots/servo-firmware
+```
+
+<TODO: describe how to do that without git>


Can we use Go templates comment syntax rather than this or the html comments below.

{{/* a comment */}}

Aha, thanks. I searched hugo's docs for what the comment syntax was, but couldn't find it!

5328e1d resolves this TODO outright; I'll changes the other comments shortly.

Turns out that {{/* a comment */}} unfortunately doesn't work for me. I suspect it might be related to these being markdown sources, though it would be an odd behaviour if the comment stripper happened after the markdown conversion.

Turns out the actual reason for this not working is that Hugo doesnt run Go's templating engine over the content files, so never strips out the comments 🤦‍♂️ D'oh!

RealOrangeOne · 2018-01-31T21:30:48Z

content/api/servo-assembly/_index.md

@@ -7,6 +7,7 @@ The Servo Assembly is used to connect to multiple types of peripheral:
 - [GPIO pins](gpio) (digital and analogue)
 - [Servos](servos)
 - [Ultrasound sensors](ultrasound)
+- [Custom Arduino commands](arduino-commands)


I'm not sure this link fits here, as this isnt a peripheral that's communicated with through the arduino.

Good point; I'd not actually read the preceding line!

After both inspecting the source and testing, it appears that what I hoped would be the case (and originally documented) is indeed the case. In robotd we call `str` on each argument before sending them over serial, so numbers (etc.) are converted to strings. The alternative I'd wondered about was integers arriving a raw integer bytes, rather than as their ASCII representation.

kierdavis · 2018-02-01T23:03:39Z

content/api/servo-assembly/arduino-commands.md

+firmware. You can get a copy of the source code from GitHub either by
+downloading the [repo archive](servo-firmware-zip) or by cloning the repo:
+
+```txt


Would bash (or similar) be a better choice than txt here?

maybe, though that causes "clone" to be highlighted a different colour. I've no idea why -- as far as I know "clone" isn't a special word in any shell. I opted for it being less distracting even if it's slightly less correct.

hmm, weird. txt is fine here in that case.

kierdavis · 2018-02-01T23:06:34Z

content/api/servo-assembly/arduino-commands.md

+
+Here's a simple command which just echoes back any arguments it receives:
+
+```cpp


We should mention that this function definition must be placed before its use (the commands array), unless they add a prototype for the function as well of course.

b93eaf0 reworks the start of the "Implementing the command" section and includes this.

kierdavis · 2018-02-01T23:08:00Z

content/api/servo-assembly/arduino-commands.md

+
+For other examples of how to define commands (and the helper functions you can
+use), you can have a look at the other commands the Arduino already supports.
+In particular the `get_version`, `led` and `servo`  are a good starting point.


Are you missing the word "commands" after "servo" here?

almost: 932d341

kierdavis · 2018-02-01T23:10:05Z

content/api/servo-assembly/arduino-commands.md

+The first argument is a quoted `String` literal containing the name you'll use
+in Python to call this command. It can contain letters, numbers and dashes (`-`).
+
+The second argument is the function you defined for your function, with an


the function defined for your function

Should this say "the name defined for your function"? (IMO "the name of your function" would probably be more straightforward anyway)

I think I meant "the function defined for your command" (de16f25); do you think that that's clear enough that we mean the function as a symbol?

We still say "name" later in the sentence, so I think that's clear enough 👍

kierdavis · 2018-02-01T23:10:15Z

content/api/servo-assembly/_index.md

@@ -8,6 +8,10 @@ The Servo Assembly is used to connect to multiple types of peripheral:
 - [Servos](servos)
 - [Ultrasound sensors](ultrasound)

+At the core of the Servo Assembly is an Arduino. If you need more direct control


Not a big deal, but is there any reason why "Arduino" here isn't hyperlinked in the same way that the occurrences in the introductions to arduino-commands.md and kit/servo-assembly/_index.md are?

Only that it didn't seem entirely useful to do so. Anyone wanting more detail will probably be on one of those pages anyway. Do we prefer to be over-linking?

Fair enough, good point.

kierdavis · 2018-02-01T23:12:55Z

content/api/servo-assembly/arduino-commands.md

+`Servo` instance. You can send strings and numbers:
+
+```python
+r.servo_board.direct_command('echo', "first argument", 2, "third argument")


Any reason for the mixing of quote types here?

'echo' is for the machine and is close to being an enum value or a slug. The others are less so, though are arguably still much more that than my usual use of double quotes (which is for text intended for humans).

They should probably all be '.

Yeah, I'd agree than in typical applications the arguments to the command won't be intended to be human-readable text - more likely to be numbers or keywords (e.g. 'low' / 'high').

kierdavis · 2018-02-01T23:16:00Z

content/api/servo-assembly/arduino-commands.md

+    At some point we might need to detail how to remove bits of the firmware
+    which the teams aren't using. That's intentionally omitted for the moment
+    as it's nontrivial to describe which bits are safe to remove.
+-->


Haven't tested this but I would imagine that removing or commenting an entry in the commands away should cause the corresponding handler function to be optimised out of the binary. This should produce some initial space savings, at least.

This does indeed seem to happen. I think I'd still rather defer this for now.

Yeah, that's something for separate PR.

kierdavis · 2018-02-01T23:18:33Z

content/api/servo-assembly/arduino-commands.md

+As your new firmware will still need to be able to talk to the Python code
+running on the Raspberry Pi, you'll need to base it on SourceBots default
+firmware. You can get a copy of the source code from GitHub either by
+downloading the [repo archive](servo-firmware-zip) or by cloning the repo:


Parentheses -> square brackets

Currently this is resulting in a broken link to https://HOSTNAME/api/servo-assembly/arduino-commands/servo-firmware-zip.

good spot: 422a0f8

Is there a way we can test that all internal links go somewhere?

I'm sure a broken link checker (e.g. broken-link-checker-local) would be a useful addition to the CI tests.

It turns out that Python will also use single quotes for strings too, so this also updates the printed return values.

This reworks these instructions to clarify where to put the function as well as provide more guidance about why some parts of the function need to be the way they are.

RealOrangeOne · 2018-02-02T08:29:00Z

content/api/servo-assembly/arduino-commands.md

+As your new firmware will still need to be able to talk to the Python code
+running on the Raspberry Pi, you'll need to base it on SourceBots default
+firmware. You can get a copy of the source code from GitHub either by
+downloading the [repo archive][servo-firmware-zip] or by cloning the repo:


Perhaps here, rather than specifically getting a zip or using git clone, we just link to the repo. This means they can not only see the source without downloading it, but the discovery path for clone / download instructions is very straightforward

RealOrangeOne · 2018-02-02T08:29:37Z

content/api/servo-assembly/arduino-commands.md

+
+You'll need to define a function to implement your command. The function should
+be placed in the `firmware.cpp` file just above the definition of the `commands`
+array (likely about like 205).


s/like/line/?

RealOrangeOne · 2018-02-02T08:31:54Z

content/api/servo-assembly/arduino-commands.md

+    as it's nontrivial to describe which bits are safe to remove.
+-->
+
+[arduino]: https://store.arduino.cc/arduino-uno-rev3


As we have done with almost all other links in the docs, can we use markdown links inline

For cases where that causes the source to wrap oddly I tend to prefer separate links. 83266fe however does this.

kierdavis · 2018-02-02T14:16:48Z

content/api/servo-assembly/arduino-commands.md

+
+You'll need to define a function to implement your command. The function should
+be placed in the `firmware.cpp` file just above the definition of the `commands`
+array (likely about line 205).


I feel like line numbers are quite a volatile way to refer to parts of the code, since changes to the file can result in many of the line numbers changing. IMO better solutions are to either copy the line verbatim here so users can ctrl+f for it, or just to say the rough location qualitatively (e.g. "about halfway through the file") - both of these methods are less likely to break when the file is modified.

I'd hope that this wouldn't change very much, also hence the "about" on the line number. Given that we do include the definition of the commands array below I don't want to repeat it here.

Do you think we need to add a comment like "see below" for this, or can we assume people will be able to read the whole of the documentation to get a full understanding?

I think we could just remove the remark - I think it should be easy enough to find commands in the source file.

I think it's useful to have the line number; I think it's clear enough that it's approximate.

kierdavis · 2018-02-02T14:18:32Z

content/api/servo-assembly/arduino-commands.md

+firmware. You can get a copy of the source code from GitHub either by
+downloading the [repo archive](servo-firmware-zip) or by cloning the repo:
+
+```txt


hmm, weird. txt is fine here in that case.

kierdavis · 2018-02-02T14:28:52Z

content/api/servo-assembly/arduino-commands.md

+on an Arduino.
+
+The following example command shows how to handle arguments from the Pi as well
+as how to return errors an other values:


"an" -> "and"

kierdavis · 2018-02-03T11:22:52Z

content/api/servo-assembly/arduino-commands.md

+In particular the `get_version`, `led` and `servo` functions are a good starting
+point.
+
+<!-- TODO should we error in robot-api/robotd if the incoming arguments include spaces? -->


This isn't really related to the docs - could we break this out into an issue on robot-api or a Trello card instead?

We'll see how this goes, if we find that this is needed (i.e: the lack of it causes problems) we can add it then.

Adimote

Adimote · 2018-02-15T12:05:39Z

content/api/servo-assembly/arduino-commands.md

+  return OK;
+}
+```
+<!-- TODO: mention being able to send back comments? -->


todo message here?

Add documentation for custom arduino commands

3265e26

This includes a summary of how to create the command on the Arduino as well as how to call it from Python and the errors which could be thrown.

PeterJCLaw requested review from kierdavis and RealOrangeOne January 31, 2018 16:08

PeterJCLaw added 2 commits January 31, 2018 16:09

Fix spellings

0b45540

Drop stray line we're not using

75ec553

I was going to use this to show how to do comments, but I think it's probably better that we don't do that for now.

PeterJCLaw force-pushed the generic-arduino-commands branch from 998e7b1 to 75ec553 Compare January 31, 2018 16:13

RealOrangeOne reviewed Jan 31, 2018

View reviewed changes

PeterJCLaw added 6 commits January 31, 2018 22:11

Explicit plaintext

79fbd56

Resolve TODO

5328e1d

Rework link to make more sense

9784c1a

Clarify which Arduino variant we ship

39e83f5

Fix spelling

69bbacd

PeterJCLaw mentioned this pull request Feb 1, 2018

Empty error messages are sometimes returned sourcebots/arduino-fw#11

Open

kierdavis reviewed Feb 1, 2018

View reviewed changes

PeterJCLaw added 5 commits February 2, 2018 00:57

Clarify what these are

932d341

Clarify this

de16f25

Fix link

422a0f8

Consistently use single quotes in Python

d1f728d

It turns out that Python will also use single quotes for strings too, so this also updates the printed return values.

Improve implementation instructions

b93eaf0

This reworks these instructions to clarify where to put the function as well as provide more guidance about why some parts of the function need to be the way they are.

RealOrangeOne reviewed Feb 2, 2018

View reviewed changes

PeterJCLaw added 4 commits February 2, 2018 12:24

Fix typo

3457afd

Move links inline

83266fe

Make the repo reference more friendly, mention needed library

e1dc43f

These are valid spellings

ac2972a

PeterJCLaw requested review from kierdavis and RealOrangeOne February 2, 2018 12:32

kierdavis reviewed Feb 2, 2018

View reviewed changes

Fix typo

d59f768

PeterJCLaw requested a review from kierdavis February 2, 2018 16:08

kierdavis reviewed Feb 3, 2018

View reviewed changes

Remove TODO

2974f73

We'll see how this goes, if we find that this is needed (i.e: the lack of it causes problems) we can add it then.

PeterJCLaw requested a review from kierdavis February 6, 2018 20:21

RealOrangeOne mentioned this pull request Feb 6, 2018

Release jan 2018 #101

Merged

Adimote approved these changes Feb 15, 2018

View reviewed changes

Adimote merged commit dc0ed32 into master Feb 15, 2018

PeterJCLaw deleted the generic-arduino-commands branch February 15, 2018 22:22

PeterJCLaw added a commit to PeterJCLaw/sourcebots-docs that referenced this pull request Mar 6, 2019

Fix sourcebots#87: make sidebar link name match page name

5f47542


		Here's a simple command which just echoes back any arguments it receives:

		```cpp

Add documentation for custom arduino commands #87

Add documentation for custom arduino commands #87

Conversation

PeterJCLaw commented Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeterJCLaw Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

RealOrangeOne Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeterJCLaw Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kierdavis Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adimote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeterJCLaw commented Jan 31, 2018 •

edited

Loading

PeterJCLaw Jan 31, 2018 •

edited

Loading

RealOrangeOne Feb 2, 2018 •

edited

Loading

PeterJCLaw Feb 2, 2018 •

edited

Loading

kierdavis Feb 2, 2018 •

edited

Loading