Skip to content
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

feat(app, app-shell-odd): add function to read csv file from usb for rtp phase2 #15403

Closed
wants to merge 44 commits into from

Conversation

koji
Copy link
Contributor

@koji koji commented Jun 12, 2024

Overview

add a function that loads csv files from an usb-drive, update protocol card & pinned protocol, and add ChooseCsvFile component (display csv files on USB).

close AUTH-460, AUTH-468, and partially AUTH-462 and AUTH-470

This PR doesn't update protocol details screen and Parametes screen. They will be implemented in a following pr since this PR's main part is displaying csv files on USB.

The following yellow background shows up when turning on the feature flag and a protocol name includes RTP.
The rendering condition part will be update in a following pr.
Screenshot 2024-06-18 at 7 24 19 PM

Screenshot 2024-06-18 at 7 12 26 PM

Confirm selection part will be implemented and a function to control radio buttons (AUTH-458)
Screenshot 2024-06-18 at 7 37 51 PM

Screenshot 2024-06-18 at 7 38 20 PM

Test Plan

  1. push this branch to a Flex
  2. select a RTP protocol that has yellow background in Protocol Dashboard screen
  3. do set up

Changelog

Review requests

Risk assessment

app-shell-odd/src/usb.ts Outdated Show resolved Hide resolved
@koji koji changed the title Feat add usb for csv file feat(app, app-shell-odd): add function to read csv file from usb for rtp phase2 Jun 14, 2024
@koji koji marked this pull request as ready for review June 18, 2024 22:30
@koji koji requested review from a team as code owners June 18, 2024 22:30
@koji koji removed the request for review from a team June 18, 2024 22:30
@koji koji marked this pull request as ready for review June 19, 2024 04:31
app-shell-odd/src/actions.ts Outdated Show resolved Hide resolved
app-shell-odd/src/actions.ts Outdated Show resolved Hide resolved
app-shell-odd/src/usb.ts Outdated Show resolved Hide resolved
Comment on lines 45 to 46
// ToDo (kk:06/12/2024) get files from the endpoint: GET /protocols/{protocolId}/dataFiles/
// return format: https://opentrons.atlassian.net/browse/AUTH-428
Copy link
Member

Choose a reason for hiding this comment

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

don't we already have api client and react api client utils for this? may as well use them!

app/src/pages/ProtocolDashboard/ProtocolCard.tsx Outdated Show resolved Hide resolved
app/src/pages/ProtocolDashboard/ProtocolCard.tsx Outdated Show resolved Hide resolved
app/src/redux/shell/types.ts Outdated Show resolved Hide resolved
app/src/redux/shell/types.ts Outdated Show resolved Hide resolved
return {
type: 'shell: ROBOT_MASS_STORAGE_DEVICE_ENUMERATED',
payload: {
rootPath: '',
Copy link
Member

Choose a reason for hiding this comment

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

rootPath being empty string seems suspicious... @sfoster1 im not fully understanding how it should be used. also does this belong in the redux/shell/update module?

@koji koji requested review from shlokamin and sfoster1 June 25, 2024 17:29
@koji
Copy link
Contributor Author

koji commented Jun 27, 2024

  • solve conflicts

@@ -47,6 +56,18 @@ const enumerateMassStorage = (path: string): Promise<string[]> =>
)
.catch(() => [])
.then(flatten)
.then(contents => {
if (filterCSV) {
const regex = /._\w/gm
Copy link
Member

Choose a reason for hiding this comment

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

can you add a comment for what this regex is searching for?

Comment on lines +189 to +191
export function registerFilePath(
dispatch: Dispatch
): (action: Action) => unknown {
Copy link
Member

Choose a reason for hiding this comment

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

i thought we were gonna reuse watchForMassStorage since it's already doing a file system watch. doesnt this sort of duplicate all of the computation?

Copy link
Member

Choose a reason for hiding this comment

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

maybe we can call this registerDataFiles or registerCsvFiles because this reducer will be searching specifically for protocol data files

setCsvFileInfo,
}: ChooseCsvFileProps): JSX.Element {
const { t } = useTranslation('protocol_setup')
const csvFilesOnUSB = useSelector(getFilePaths).payload.filePaths ?? []
Copy link
Member

Choose a reason for hiding this comment

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

is getFilePaths an action creator or a selector? if its meant to be a selector, we need to rework it to take in state and return file paths (rather than an action)

Comment on lines +129 to +131
export const getFilePaths = (state: State): SendFilePathsAction => ({
type: SEND_FILE_PATHS,
payload: { filePaths: state.shell.filePaths },
Copy link
Member

Choose a reason for hiding this comment

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

is this meant to be a selector or an action?

Comment on lines +195 to +199
void enumerateMassStorage(action.payload.rootPath, true).then(
csvFilePaths => {
dispatch(sendFilePaths(csvFilePaths))
}
)
Copy link
Member

Choose a reason for hiding this comment

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

in the system update reducer, we respond to shell:ROBOT_MASS_STORAGE_DEVICE_ENUMERATED and filter out all of the provided file paths with ONLY relevant update files (system zips).

in the usb case, can we do something similar? we could respond to shell:ROBOT_MASS_STORAGE_DEVICE_ENUMERATED (like we already are), but have a special function (similar to getLatestMassStorageUpdateFiles that only grabs CSV files for us. that way we can draw a distinction between getting ALL files in the usb dir vs specific files relevant to a particular redux module (system update, or csv files)

Copy link
Member

Choose a reason for hiding this comment

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

something like getLatestMassStorageCsvFiles or getLatestMassStorageUpdateFiles.

and that function's job is just to filter for relevant CSV files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

trying the above method, but action.payload.filePaths is empty - [] without calling enumerateMassStorage.

Copy link
Member

Choose a reason for hiding this comment

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

robotMassStorageDeviceEnumerated gets called by enumerateMassStorage, which means that as long as you respond to an action of type ROBOT_MASS_STORAGE_DEVICE_ENUMERATED by calling this new getLatestMassStorageCsvFiles function you should be good.

is the action of type ROBOT_MASS_STORAGE_DEVICE_ENUMERATED getting dispatched?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes
rootPath has the path but filePaths don't have any path.

unplug the flash drive from Flex -> plug into the flash drive on Flex

Jun 28 21:12:10 Flex1234567890 opentrons-robot-app.sh[5458]: Mass storage device /media/sda1 removed but this was not an update source
Jun 28 21:12:16 Flex1234567890 opentrons-robot-app.sh[5458]: New mass storage device sda1 detected
Jun 28 21:12:17 Flex1234567890 opentrons-robot-app.sh[5458]: shell:ROBOT_MASS_STORAGE_DEVICE_ENUMERATED []
Jun 28 21:12:17 Flex1234567890 opentrons-robot-app.sh[5458]: no updates found in mass storage device

@koji
Copy link
Contributor Author

koji commented Jul 1, 2024

this will be taken over by #15551

@koji koji closed this Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants