Flesh out basic structure of API

trickeydan commented 4 years ago

Based on https://docs.google.com/spreadsheets/d/15hQWcMPxllfsKocRPlKsLv_q2vKkzzNmb2wYxA_VBTc/edit#gid=556274040

PeterJCLaw commented 4 years ago

Could you make that document open to at least @srobo accounts? (Ideally everyone, unless there's something secret about it?)

trickeydan commented 4 years ago

Could you make that document open to at least @srobo accounts? (Ideally everyone, unless there's something secret about it?)

Have updated to "Anyone with the link can view". You are probably interested in the "sr-robot vs. sr-robot3" sheet.

This structure was discussed in a brain team meeting on 2nd July, unfortunately no minutes are available.

PeterJCLaw commented 4 years ago

Some technical thoughts here:

It's great to see innovation around some things where the SR API isn't the most ergonomic. Being able to assign BRAKE or COAST mode for motors more easily at each point of use feels like a good iteration.
There are a number of aspects of the new API design which couple it to specific ways that we currently do things. The most obvious is perhaps changing mode from a str to an is_competition bool. I realise that this is perhaps a result of a desire for a form of type safety, however it creates a hard limitation that we now only have one and a half modes -- "competition" and "not competition", closing off the options of having other modes entirely. Another example is introducing singular forms of many of the board names, which bakes in the assumption that the kit won't ever have multiple servo boards.
The API couples the python implementation to the details of the physical boards that we currently have. The most obvious example is R.power.beep() moving to R.power_board.piezo.buzz(). This means that if the beeping ability ever changed, perhaps to a speaker or some other form of buzzer, there would need to be a corresponding API change. In turn, this makes development & deployment of kit more complex as any code is likely to be strongly coupled to a specific configuration. Again this is something which the current API design does not suffer from -- we have upgraded much of the kit without changing the API. This also feels like a level of detail which just isn't necessary for competitors (who may still be learning Python) to think about.

Separately, on the topic of breaking changes to the API in general.

This feels likely to create confusion among mentors and/or result in them accidentally misleading competitors. Mentors are much more likely to go from memory of the API than check the docs each time, and that's what we want -- it's much faster for them to have learned our API once than need to re-learn it on each encounter. We should bear in mind that many of our mentors (including a good number who only come to events) have multiple years of experience with our kits, so preserving their ability to transfer knowledge from year to year is valuable. Their time is possibly our most precious resource.

The SR API has been stable and well proven for a long time, so it feels like we should have a strong reason to change it (and no, I don't consider the fact that it's being reimplemented as a strong reason to change the public API).

While the technical sides of the API are primarily something for the kit team, as noted above there is a considerable impact on mentors here which must be considered and creates overlap with the competition team as well as other aspects of SR.

It would be good to at least understand the reasoning here, though I would encourage that wider input should be sought before committing to breaking changes.

trickeydan commented 4 years ago

Thanks for your input on this @PeterJCLaw. I understand your concerns and will ensure that they are raised at our next meeting. I'm going to have a think about how we can open up input on the changes from our network of mentors to help us evaluate the impact that any changes may have.

Some quick notes on your comments, although I'm unsure if this is the best place to post it.

changing mode from a str to an is_competition bool

You make a good point on this, I'll raise this for reconsideration. I'd rather move away from the str still, as you correctly guess, for type safety reasons. I suspect that an Enum would be sufficient here, and provide API compatibility with the previous DEV and COMP constants.

introducing singular forms of many of the board names, which bakes in the assumption that the kit won't ever have multiple servo boards.

I think you may have misunderstood the document, it was originally intended to be internal to the Brain Team anyway. There is definitely not an assumption about the number of servo boards here. This change is mainly aimed at removing the integer indices for the board "lists", replacing them with dict-style BoardGroup objects. Whilst the integer indexing in sr-robot is deterministic (with the exact same set of boards), there are a number of reasons to deprecate this anyway (side note: j5 used to support integer indices for BoardGroup objects).

The r.servo_board singular form makes use of the BoardGroup.singular() function, which throws an error if there isn't exactly one of the given board. In the circumstance that multiple servo boards are connected, the r.servo_boards attribute can be used with the board indexed by serial number: r.servo_boards["srHU8k"]

The API couples the python implementation to the details of the physical boards that we currently have.

we have upgraded much of the kit without changing the API

I agree that it is coupled to the physical boards that we are using. However, this has been the case for the API for previous kit revisions, including the migrations between them. (Gotta love the yield stuff in the very early APIs.) For example when the JointIO board was replaced, the API also changed. Using r.power.beep assumes that the speaker / buzzer component remains on the power board, and that we even have a power board; I think that making an API that is agnostic to all physical aspects of the kit would be both very breaking and would result in an unintuitive API.

PeterJCLaw commented 4 years ago

Thanks for your reply, glad to help.

changing mode from a str to an is_competition bool

:+1: to using enums instead.

For clarity: I'm not saying that we cannot make changes where we can improve (I agree that moving from str to an enum is a strict improvement), but rather that we should be clear on the motivation and ensure we don't create confusion.

An example of a specific place I foresee confusion is in moving the digital_read method down a layer. The apparent similarity here will make it feel like nothing's changed, yet its place in the API and signature change will create breakage which will then be potentially annoying to debug. It's not clear to me that this is worth the cost of breaking the API.

introducing singular forms of many of the board names, which bakes in the assumption that the kit won't ever have multiple servo boards.

I think you may have misunderstood the document, it was originally intended to be internal to the Brain Team anyway.

Are you suggesting that the API in fact won't have the singular forms?

The API couples the python implementation to the details of the physical boards that we currently have.

we have upgraded much of the kit without changing the API

I agree that it is coupled to the physical boards that we are using. However, this has been the case for the API for previous kit revisions, including the migrations between them.

I realise it's not perfect (and agree that R.power.beep() is already leaning in this direction anyway), and I'm not at all suggesting that we try to mush everything onto the Robot class.

I do still think there's a useful distinction to have between the physical properties of a board a the capabilities of the board. To me the fact that we use a piezo to do the beeping is an implementation detail of the power board. Ignoring for a moment the fact that a kit always needs a power board, the fact that the buzzer is on the power board isn't an implementation detail -- if the kit didn't have that board present, the buzzer wouldn't be available. Similarly it's necessary to expose the servo board > servo channel layers, but we don't for example expose the difference between the high and low power servo outputs. Perhaps this is a more useful framing for my concern?

Something else which occurred to me with regards to breaking changes in the API -- we now have a simulator (two in fact) which is being developed by the Competition Team (no idea where it'll end up long term). It is currently assumed that we might use the (webots) simulator in future years at least as a supplement to the physical kits or possibly as another competition element (as yet undefined). Currently the simulator API is very compatible with existing code (Antione ran his own old robot code in it (via lib2to3) without issue at one point), so changes to the robot API will create work in both updating the simulator (and any house robots which have been developed for it).

trickeydan commented 4 years ago

we should be clear on the motivation and ensure we don't create confusion

I personally 100% agree on this. Impacts of any changes will be considered. Migration guides for competitors and mentors will be produced, (hopefully) along with an API "Quick Reference" too.

(Although I personally think that we should encourage competitors to rewrite much of their code between years, as it enables Y12 competitors to learn to code also)

I think you may have misunderstood the document, it was originally intended to be internal to the Brain Team anyway.

Are you suggesting that the API in fact won't have the singular forms?

Apologies for the ambiguity here :facepalm:, the document was originally intended to be internal to the Brain Team. Hence it is not clear on everything as there was a discussion over Google Meet as we put it together. I somewhat wish we had minuted it now. :shrug:

The singular forms are intended to be present in the API, and actually should work with the repo in the current state. The singular forms are primarily intended so that competitors do not need to look up the serial number for a board if they only have one of that board type connected.

Feel free to try it out in its current state, although I'm afraid that there is a lack of docs for it so far :disappointed:. (The sbot / sourcebots docs are similar, but not identical at this time)

With a physical kit:

from sr.robot3 import *

r = Robot(auto_start=False)  # wait_start isn't implemented yet

r.servo_board.servos[0].position = 0.7

r.servo_boards["srY6I8"].servos[0].position = 0.2

Alternatively, if you don't have a kit:

from sr.robot3 import *
from sr.robot3.env import CONSOLE_ENVIRONMENT

r = Robot(env=CONSOLE_ENVIRONMENT, auto_start=False)  # wait_start isn't implemented yet

r.servo_board.servos[0].position = 0.7

r.servo_boards["SERIAL"].servos[0].position = 0.2

I do still think there's a useful distinction to have between the physical properties of a board a the capabilities of the board. To me the fact that we use a piezo to do the beeping is an implementation detail of the power board. Ignoring for a moment the fact that a kit always needs a power board, the fact that the buzzer is on the power board isn't an implementation detail -- if the kit didn't have that board present, the buzzer wouldn't be available. Similarly it's necessary to expose the servo board > servo channel layers, but we don't for example expose the difference between the high and low power servo outputs. Perhaps this is a more useful framing for my concern?

I will make sure that these concerns are considered. :slightly_smiling_face: I'm no longer sure what my position is on this.

With regards to the simulator(s), the responsibility for maintenance lies with the Dev Tooling Team, a subset of the Kit Team. This team currently doesn't exist, although it will do shortly :tada:.

Currently the simulator API is very compatible with existing code (Antione ran his own old robot code in it (via lib2to3)

The migration to Python 3 for the webots simulator was not done with the explicit support of the kit team / brain team, and I specifically advised that it should remain on Python 2 (as we were planning breaking changes with the Python 3 upgrades that we were planning for SR2021).

When the Dev Tooling Team is formed, I will make sure that this issue is raised to them. I suspect that the IDE may get attention from them before the simulator does.

I suggest that when the SR2021 competition team is formed, they have a meeting with the kit team to discuss requirements for the upcoming competition cycle, which will include simulator support etc.

On a similar note to this discussion, I am advised that there is likely to be breaking changes to the conventions used within the vision API for rotations and position. This would be the result of a move from libkoki to something new (possibly based on Zoloto), although no decisions have been made here yet (and are out of scope for this issue, recommend continuing in slack if you want to ask about this).

I'd like to clarify that I do appreciate your input on all of this, and would be happy to discuss any changes with you over a video call if that would help. :slightly_smiling_face:

trickeydan commented 4 years ago

+1 to using enums instead.

For clarity: I'm not saying that we cannot make changes where we can improve (I agree that moving from str to an enum is a strict improvement), but rather that we should be clear on the motivation and ensure we don't create confusion.

--> #8

trickeydan commented 3 years ago

The API structure was agreed.

srobo / sr-robot3-j5

Flesh out basic structure of API #1