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

Sensor Test Cleanup #8

Merged
merged 7 commits into from
Apr 2, 2024
Merged

Sensor Test Cleanup #8

merged 7 commits into from
Apr 2, 2024

Conversation

fchorney
Copy link
Collaborator

@fchorney fchorney commented Apr 2, 2024

Massive clean up of sensor_test.ts

  • Cleanup of logs, code, documentation
  • Use enums for Panel and Sensor array positions
  • Clamp/Scale sensor values based on mode
  • Attempting to get rid of parsing errors

Also it looks like requesting the config has been broken somehow. I'll get that working again after this. I want to go through the config code and clean that up too.

ui/pad-coms.ts Outdated
Comment on lines 78 to 80
if (response[0] !== API_COMMAND.GET_SENSOR_TEST_DATA) {
return null;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This just kind of speaks to needing to move to a system where we handle a packet based on its first byte rather than expecting the packet we get after requesting one to be the correct response.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, definitely something to handle soon. I think this will actually be easy to do inside the sdk where we can just not resolve the promise right away and keep watching for more packets until we see the response that we're looking for.

Comment on lines +36 to +38
if (data) {
setTestData(data);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This seems to work for the most part, but I still get some console errors when I interact with the UI buttons for sending test data and I'm not too sure why.

Copy link
Owner

Choose a reason for hiding this comment

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

I think with the way things are currently setup, we're always going to get some weird in-fighting between the test command buttons and the automatic reads happening here. It won't be a problem in the long term, so this should be fine to just ignore.


/**
* Sensor Test Mode values the stages expect
*/
export const SENSOR_TEST_MODE = {
export enum SensorTestMode {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Converted this to an Enum. Not sure it matters too much, I think they just end up just being consts anyway right? I like the idea of assigning an argument as an Enum type instead of just saying number

Copy link
Owner

Choose a reason for hiding this comment

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

yeah... they kind of become consts, but not really. they're honestly a weird relic of the early days of typescript and there are some foot-gun type properties about them. this is fine for our purposes though.

Comment on lines +143 to +151
* In Noise mode, we receive standard deviation values squared.
* Display the square root, since the panels don't do this for us.
* This makes the number different than the configured value
* (square it to convert back), but without this we display a bunch
* of four and five digit numbers that are too hard to read.
*
* TODO: Do we want to round this value or just display decimal values?
*/
return Math.sqrt(value);
Copy link
Owner

Choose a reason for hiding this comment

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

We can also resolve this by just displaying the squared values as they come across the wire in a more abbreviated form e.g. 12k instead of 12345. I'm not even sure what the value in the noise test mode is, so we can wait to decide later.

Comment on lines +154 to +156
// TODO: We need a way to pass in if the stage we are getting this data for
// is using FSRs or not. Defined as `true` for now.
const isFSR = true;
Copy link
Owner

Choose a reason for hiding this comment

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

we gotta become good friends with someone on gen1 pads (werd) or convince zewt to actually come over here and implement support for us because... 😓

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The config will tell us if it's an fsr or not. We just need to get that in here, but also yeah maybe werd can help us test things out. There's a chunk of old config or load cell stuff that we just can't test.

@noahm noahm merged commit a54856e into main Apr 2, 2024
4 checks passed
@noahm noahm deleted the fc/docs-and-cleanup branch April 2, 2024 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants