API review

Proposer: Curt Meyers

Present at review:

• List reviewers

Question / concerns / comments

Enter your thoughts on the API and any questions / concerns you have here. Please sign your name. Anything you want to address in the API review should be marked down here before the start of the meeting.

• Josh

  • BatteryState2

    • fields are named wrong according to our standards. Should be last_time_battery, bat_reg, etc.
    • I have no idea what each of the fields means, as there are no comments in the message. Also, the "epoch time" comment is wrong.

  • BatteryServer2

    • again, fields are named camelCase instead of underscored
    • again I have no idea what any of the fields mean.

    • Why are there bool[4] arrays for each battery(?), and a variable-sized array for the batteries themselves? Why aren't those bools inside the BatteryState2 message?

Tully

BatteryState2

• There are still no units or links in the message documentation, I don't know where to find the units on the wiki from the message documentation.

BatteryServer2

• Time left should be a duration and durations are measured in seconds
• Why is percent charge an int32? What exactly are the units?
• wtf is reserved? if is has meaning is should be documented if it doesn't it should be removed.

• If they are all bool[4]s should they be BatteryState2[4]?

Follow-Up

• Curt
  • changed the variable names to conform.
  • Added comments for everything, you have to read the vendors documents for further descriptions. There are links on the wiki.
  • The question about the flags that pertain to each battery being moved into the batteries own message. The reason it is done this way is because these flags come from the server board and are independent of the battery existing, being on-line or functional. It can be moved, dynamically sized and so on, I was just trying to keep this as a close mirror to the layout of the hardware so someone that reads the documentation might be able to follow what is going on.
  • Changed time_left to a duration type, yes it is in seconds with a 1 minute resolution.
  • Percent is an int32 because I was asked to use language friendly/agnostic data types. It is 0 to 100%.
  • removed reserved. I don't know what it is but thought it might be interesting to log. It is gone now.

  • Since no one needs to care about the hardware layout I just moved the boolean flags into the BatteryState object.

  • The thing that generates the code from the message descriptions doesn't work if you declare a static size for an array of an object, the C code produced is wrong. Maybe that should be fixed, this my solution, I resize the vector in the parents constructor to 4.

Meeting agenda

To be filled out by proposer based on comments gathered during API review period

Conclusion

Package status change mark change manifest)

• Action items that need to be taken.

• Major issues that need to be resolved