Skip to content

Automatic test#17802 #26

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Automatic test#17802 #26

wants to merge 22 commits into from

Conversation

MPIDavidLiu
Copy link
Contributor

[python script] update the missing API for python package

@MPIDavidLiu MPIDavidLiu added the enhancement New feature or request label Apr 11, 2025
@MPIDavidLiu MPIDavidLiu self-assigned this Apr 11, 2025
Copy link
Contributor

@beltoforion beltoforion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reduce the size of future PR and split them into smaller chunks.

@@ -187,13 +186,11 @@ def start_prepare_station(self, station: LoaderStation, angle: float | None = No
return Response.check_resp(self.comm.read_line())


@deprecated("duplicate functionality; Use SentioProber.move_chuck_work_area!")
def switch_work_area(self, area: str):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is a duplication of the fiunctionality of move_chuck_work_area. Its name violates the rule that the python api names should match remote command names. Therefore it has been marked as deprecated.

Is there a reason to remove the deprecation marker?


def set_chuck_thermo_energy_mode(self, mode: str) -> Response:
def set_chuck_thermo_energy_mode(self, mode: Union[str, ChuckThermoEnergyMode]) -> Response:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to keep the python API simple and unambiguous we should be as clear in the types of our input parameters as possible. Therefore we should only except enums if we have an enumerator that contains all available options.

I have filed a CR ticket: https://ast-svn.mpi.com.tw/redmine/issues/18176

We do not want the scripts on the client site to diverge in order to keep out code maintainable,

return Response.check_resp(self.comm.read_line())

def set_chuck_thermo_hold_mode(self, mode: bool) -> Response:
def set_chuck_thermo_hold_mode(self, mode: Union[bool, ChuckThermoHoldMode, str]) -> Response:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not return a response object because it is not an async function,

"""
self.comm.send("qal:get_calibration_status")
resp = Response.check_resp(self.comm.read_line())
return resp.message()
status = resp.message().strip()
if status.upper() != "OK":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think this logic ever triggers because get_calibration status will return an error code if something is off. At least if the remote command specification is correct.

In general i would oppose parsing remote command response strings to determine wether an action failed or not because this will break if someone in the future decides that SENTIO will return something alse then "ok". Please rely on error codes.

"""
Clear network data for a DUT.

Wraps SENTIO's "qal:clear_dut_network" remote command.

Args:
dut_name: The name of the DUT (e.g. "RefDUT").
drift_type: The type of drift data to clear ("DriftRef" or "Drift").
drift_type: The type of drift data to clear (DriftType.DriftRef or DriftType.Drift).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please specify the type as requested by google docstyle format: "drift_type (DriftType)" . The system we are using to create the documentation needs this information to create the proper links in the API documentation.

return Response.check_resp(self.comm.read_line())

def set_signal_tower_buzzer(self, state: int) -> Response:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not have a return type

return Response.check_resp(self.comm.read_line())

def start_move_indexer_pos(self, pos: int) -> Response:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not have a return type

return Response.check_resp(self.comm.read_line())

def swap_bridge(self, side: str, device_position: str = None) -> Response:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not have a return type

return closed == "1", locked == "1"

def set_door_lock(self, door: str, lock: bool) -> Response:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not have a return type

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not review the test code; The PR is too large therfore my Focus is on production code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants