Automatic test#17802#21
Conversation
…nto add_new_command_API
…ntioProberDev/SentioProberControl into David.Liu/AutomaticTest#17802
Add new command api
beltoforion
left a comment
beltoforion
left a comment
There was a problem hiding this comment.
This is too much to review. Can you look at the comments and split it into multiple smaller reviews?
The main issues are that the command parameters or response values are ofteh too complicated and deviate too much from the underlying remote command. Please use existing enumerations in command parameters and responses wherever possible,
| resp = Response.check_resp(self.comm.read_line()) | ||
| return resp.message() | ||
|
|
||
| def query_wafer_id(self,station : LoaderStation, slot : int) -> None: |
There was a problem hiding this comment.
Return type specification should be string
| resp = Response.check_resp(self.comm.read_line()) | ||
| return resp.message() | ||
|
|
||
| def read_wafer_id(self,angle : str, side : str) -> None: |
There was a problem hiding this comment.
return type should be str
| resp = Response.check_resp(self.comm.read_line()) | ||
| return resp.message() | ||
|
|
||
| def start_prepare_wafer(self,station : LoaderStation, slot : int, angle : int, readid : int, unloadstation : LoaderStation, unloadslot : int) -> None: |
There was a problem hiding this comment.
Since this command is starting an async SENTIO command return type should be RemoteCommandResponse. Async commands are the only type of commands that should return RemoteCOmmandResponse because the command may be needed for waitcomplete.
| # ------------------------------------------------------------------------- | ||
| # 1) retrieve_substrate_data | ||
| # ------------------------------------------------------------------------- | ||
| def retrieve_substrate_data(self, site: Optional[str] = None) -> Tuple[int, List[str]]: |
There was a problem hiding this comment.
The retur type should be List[ChuckSite]. Returning the number is not necessary when this number always corresponds to the size of the list which should be the case here.
If we return a list of string the client would still need to parse the result from the string to make sense of the information. The next step in the client could be a foreach loop over all the aux sites but returning a string makes this more difficult.
| # ------------------------------------------------------------------------- | ||
| # 2) get_substrate_type | ||
| # ------------------------------------------------------------------------- | ||
| def get_substrate_type(self, site: Optional[str] = None) -> str: |
There was a problem hiding this comment.
Second input parameter should be of type chuck site. An exception should be thrown if the site is not an aux site.
| # ------------------------------------------------------------------------- | ||
| def get_element_life_time( | ||
| self, | ||
| element_standard_id: str, |
There was a problem hiding this comment.
Parameter should be Union[SubstrateType, ChuckSite] or Union[string,ChuckSIte], depending on wether we should have a SubstrateType enumerator
| self, | ||
| element_standard_id: str, | ||
| site: Optional[str] = None | ||
| ) -> Dict[str, Union[str, float, int]]: |
There was a problem hiding this comment.
Return type should be tuple similar to remote command return type. This is too hard to unpack.
| Response.check_resp(self.comm.read_line()) | ||
|
|
||
| resp = Response.check_resp(self.comm.read_line()) | ||
| return resp.message() |
There was a problem hiding this comment.
The remote command doc says the return value if either "ok" of a wafer "id". We should never ever return "ok" because a client will not know that this is not a wafer id. If SENTIO does not give a wafer id an empty string should be returned.
| Response.check_resp(self.comm.read_line()) | ||
|
|
||
| resp = Response.check_resp(self.comm.read_line()) | ||
| return resp.message() |
There was a problem hiding this comment.
From the remote command documentation i cannot wee that prealign would ever return something other than "OK".
| resp = Response.check_resp(self.comm.read_line()) | ||
| return resp.message() | ||
|
|
||
| def start_prepare_station(self, station: LoaderStation, angle: float | None = None) -> None: |
There was a problem hiding this comment.
This should return a RemoteCommandResponse object since it wrapping an async remote command.
4 participants
Add the lost remote command and the remote command unit test