Severson-Group / AMDS

Voltage and Current Sense Board
BSD 3-Clause "New" or "Revised" License
2 stars 1 forks source link

Formalize AMDS release approach and revise history #51

Open elsevers opened 1 month ago

elsevers commented 1 month ago

@codecubepi , @npetersen2 -- collecting a plan on how to handle AMDS releases:

Versioning Approach

Per this comment lets plan to

Revise history

Looking through the AMDS commit history I suggest the following:

  1. We create a V1.0.0 release by branching off of 3e2b936 with a commit that adds a changelog. We create a V1.0.x branch pointing to this commit.
  2. We merge the v1.0.x branch into develop (which has the new code necessary for the timing manager since #48 was merged)
  3. We proceed with a v2.0.0 release: create a new commit on develop that adds a changelog entry, create a v2.0.x branch here. (note that this is indeed a major release (v2.0.0) because the PR #48 adds a feature that changes the interface to the AMDC)
  4. We delete the v1.3.x tag / release (because now we have the v2.0.0 release)
  5. We create a new section near the top of AMDS Interface doc of what version of AMDS is required for each release of AMDC. I think it will be a table with row 1 showing AMDC v1.0.x - v1.2.x | AMDS >= 1.0; AMDC 1.3.x | AMDS >= 2.0

(We'll set up default branches and branch protection rules in AMDS just like AMDC to point to v1.1.x etc)

elsevers commented 1 month ago

@codecubepi , @npetersen2 -- one question: with the latest upgrades for the timing manager (#48), is the current AMDS code still compatible with AMDC v1.2 and lower?

codecubepi commented 1 month ago

codecubepi , npetersen2 -- one question: with the latest upgrades for the timing manager (#48), is the current AMDS code still compatible with AMDC v1.2 and lower?

Umm... maybe? The old AMDC Firmware (pre-v1.3) sends two interrupts to the AMDS: one tells it to sample its sensors (SYNC_ADC), the second tells it to return the latest data (SYNC_TX). With the Timing Manager introduced in v1.3, the idea is that a sensor interface receives one signal from the TM telling it to sample data, and once data is sampled, it is assumed that it should be returned immediately. It's a different philosophy of data collection. Therefore in the new AMDS code, the SYNC_TX interrupt has been removed, as the AMDS will both collect and return data with each SYNC_ADC interrupt.

In the old AMDC-Firmware, SYNC_ADC is already aligned with the PWM carrier. With the new AMDS code sending back the data automatically with each occurence of the SYNC_ADC signal, you would have to make sure the v1.2 AMDC driver is expecting the data. So it may be possible if a user can guarantee that their control code is calling motherboard_request_new_data() in sync with the PWM carrier and SYNC_ADC signal. But without the Timing Manager to synchronize the control code to the PWM carrier (and by extension the SYNC_ADC signal), I'm not sure how this guarantee could be made. Note that this hasn't been tested, it's just my speculating.

elsevers commented 1 month ago

Thanks @codecubepi. So this brings up a question:

in the AMDC, vA.B.C:

If we employ the same logic to the ~AMDC~ AMDS, is this newest release actually v2.0.0 because it potentially breaks user code that is running on the AMDC?

Maybe the counter argument is that if a user upgrades both the AMDC and the AMDS at the same time, their user code stays the same and doesn't need changing?

npetersen2 commented 1 month ago

I agree, it is a bit hairy to say the v1.3.0 release is really a "minor release" but I argue it actually is:

Apart from some function renaming (which yes, theoretically pushes it to a major release, but in practice, is just fixing silly names from years past), the v1.3.0 is indeed backwards-compatible for users. Users can download the new code, and their existing user code should work as before.

The difference here is that, the AMDS changed, which is a separate system....so, really, this new AMDS code should be v2 and the old AMDS code should be v1. And, in the AMDC firmware, the new v1.3.0 simply requires >= v2 while the previous AMDC firmware (i.e., v1.2.0 and before) require < v2 for the AMDS

elsevers commented 1 month ago

Thanks! I screwed up my AMDC and AMDS labels in my question, but you answered what I was really wondering anyway:

This new release of the AMDS should be v2.0.0. I'll update the issue description accordingly.