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

Sbee dev #203

Merged
merged 10 commits into from
Jul 6, 2023
Merged

Sbee dev #203

merged 10 commits into from
Jul 6, 2023

Conversation

samiebee43
Copy link
Contributor

@samiebee43 samiebee43 commented Jun 28, 2023

Changes

Acceptance Criteria

  • Boot ✅✅
  • Get Spectra ✅✅
  • Switch to Hardware page, correct string shown next to [Save .ini File] ✅
  • Debug Gain visibility always True, set gain persist after restart ✅✅
  • Save measurement, to disk, and display in 'Clipboard/Measurements' ✅
  • Load measurement from disk ✅

Two checkmarks means another check after merging main

@samiebee43 samiebee43 marked this pull request as ready for review July 5, 2023 18:39
@samiebee43 samiebee43 requested a review from mzieg July 5, 2023 18:39
Copy link
Member

@mzieg mzieg left a comment

Choose a reason for hiding this comment

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

I had to fix two of my own bugs to test this (honestly not sure how that happened), but I agree it works well.

multispec = ctl.multispec,
slider = sfu.slider_gain,
spinbox = sfu.doubleSpinBox_gain)
ctl.gain_db_feature = GainDBFeature(ctl = ctl)
Copy link
Member

Choose a reason for hiding this comment

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

I basically disagree with the premise that the application is improved by making everything global (every business object has access to basically everything in the application, including accessing every GUI widget through a flat namespace), but I'm willing to accept it. You can quote all the PEPs you want — I just place a higher priority on encapsulation. Anyway, I'm not fighting it, and I do believe that the primary maintainer of a codebase has the right to move things around to suit their preferences (I did). I will try to learn and follow your style so we're contributing to a consistent design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think of it as a lesser of two evils. I lean towards 'the ctl' way because it makes it easier to track references via grep or static analysis, and semantic changes can be performed faster.

import logging
import enlighten
Copy link
Member

Choose a reason for hiding this comment

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

What does this line add?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It enables static tracing using the annotation on ln43.

Copy link
Member

Choose a reason for hiding this comment

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

Ah...okay

@samiebee43 samiebee43 merged commit 67290a8 into main Jul 6, 2023
@samiebee43
Copy link
Contributor Author

TODO: "persistent GainDB via .ini" >> changelog

Also this shadows the device's 'default GainDB' eeprom setting. So the only time it gets loaded from eeprom is the first time this spec is connected to a particular computer. This isn't necessarily a problem, as it saves us unnecessary writes to the EEPROM, but it can be categorized as kind of weird default behavior-- and it does not support gain persistence across multiple computers, obviously.

@mzieg
Copy link
Member

mzieg commented Jul 7, 2023

changelog

It's in there.

this shadows the device's 'default GainDB' eeprom setting

Yeah. So, history:

EEPROM availability: originally, Wasatch spectrometers lacked an EEPROM, so the .ini file was literally where such things were mastered. Then we added the EEPROM, but still had lots of spectrometers in the field without EEPROM, so we were kind of maintaining both. Then we dropped ENLIGHTEN support for the old (non-FID) spectrometers, so "nearly all" spectrometers now have EEPROMs (*XL).

EEPROM fields: the "startup_" EEPROM fields, supporting initial integration time, gain, and TEC setpoint are relatively new. So we were still very much using the .ini to let users configure the "normal, intended" settings for that unit. Now we have the startup fields, but as you say EEPROMs have a limited write lifetime.

Scalable configurability: we don't necessarily want to let all users configure all settings...some settings can lead to degraded measurement quality, others could potentially infringe safety, others could even damage equipment. Some users are highly-qualified scientist/engineers, some are station operators. The thought was that we would roughly group settings into 3 categories: 1. those anyone could configure in the .ini, 2. those OEM/advanced users could configure in the EEPROM ("edittable"), and 3. production/"everything". And now we've stirred that pot again by adding "Expert Mode," which would be about a 1.5 (e.g. can fiddle excitation on-screen, but still not write EEPROM).

So yeah, this is a sliding scale across multiple dimensions with multiple vocal stakeholders, and we're trying to maintain a balance across different use-cases.

In this particular case, the EEPROM startup fields are arguably more important for people not using ENLIGHTEN (OEM integrators who want the unit to startup and operate in a specific mode on power-up).

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.

Invalid path shown next to Save .ini button
2 participants