TobiasBales / PlayuavOSD

A Graphical OSD for FPV
http://en.playuav.com
GNU General Public License v3.0
22 stars 15 forks source link

Version splash screen and related EEPROM variables #30

Closed StewLG closed 8 years ago

StewLG commented 8 years ago

This is a boring but functional version splash screen that appears once, when PlayUAV boots. This should help users become more aware what version they are running, and to encourage developers to have some discipline about setting such things.

New EEPROM variables:

PLAYUAV_VERSION_MAJOR PLAYUAV_VERSION_MINOR PLAYUAV_VERSION_REVISION

These correspond to a version number like 1.2.4, or major.minor.revision. These numbers are #defined (currently), and are a hand edit, but we could certainly make something in the build that at least auto-bumps revision for us, although that's not handled in this yet.

These version numbers get stored in the EEPROM, and whenever PlayUAV starts it checks for a mismatch. If the version numbers differ, it sets the version numbers for the firmware currently loaded. This should allow the configurator to query the board and figure out what firmware it is running.

The last EEPROM variable is version_splash_milliseconds_to_show. This defaults to 5000, or 5 seconds. If the user sets it to 0, the splash screen won't be shown at all. The least we need to do is support setting this in the configurator; firmware version detection and so forth would of course be great.

Versioning starts at 1.1.1; that appears to be the next version after Tom Ren's.

img_7455

TobiasBales commented 8 years ago

I think I would prefer a define in the form of `#define VERSION "1.1.1", and on top of that, not store it in eeprom at all, rather add an option to get that value from the board (not via get params, but another operation entirely). When storing it in eeprom, one would have to (ideally) reload the config after flashing, to make sure, that the config tool keeps the correct version when flashing etc.

This can be avoided by keeping it seperate from the eeprom.

As for string vs 3 ints, I am not that fixed on it, think it might just be easier to handle overall, but your feedback is more than welcome.

Regarding the eeprom, sorry if I got my thoughts across not clearly earlier.

StewLG commented 8 years ago

I at first considered having the version as a string, and certainly could do that, I was just trying to make things at bit more rigid so we could have build numbers incremented by tooling without getting into string parsing. Again, could be changed.

StewLG commented 8 years ago

As for storing the version number in the EEPROM, if you don't store it there, how can the configurator figure out what version of the firmware the board is running? I'm assuming that's something we'd like to do eventually, so that the configurator can say "I see you are running 1.1.1. There's a new version, 1.2.4 available. Would you like to install it?" If it isn't in the EEPROM, what's the mechanism for the configurator discovering what version is installed on the board?

You're right, the board will have to reboot for the EEPROM to be read, but that happens immediately and automatically after an upload of the firmware in the configurator. It only needs to happen once for the EEPROM to agree with the firmware version that's actually loaded, so I don't see what issue or use case you are worried about.

StewLG commented 8 years ago

It occurs to me maybe you've missed the flow, or I've described it badly, so I'll outline it here.

The version numbers start as #defines in the firmware:

**// Version number: major.minor.revision (1.2.6 for example)

define PLAYUAV_VERSION_MAJOR 1

define PLAYUAV_VERSION_MINOR 1

define PLAYUAV_VERSION_REVISION 1**

These #defines are directly used for the version splash screen display:

sprintf(version_str, "PLAYUAV V%d.%d.%d", PLAYUAV_VERSION_MAJOR, PLAYUAV_VERSION_MINOR, PLAYUAV_VERSION_REVISION);

So the firmware is 100% in control of what's displayed to the user, and it has nothing to do with EEPROM.

During boot, this always happens:

void setVersionNumberInParams() {
// If value of PlayUAV version written into EEPROM does not match the actually // loaded PlayUAV firmware file.. if ( (eeprom_buffer.params.Version_major != PLAYUAV_VERSION_MAJOR) || (eeprom_buffer.params.Version_minor != PLAYUAV_VERSION_MINOR) || (eeprom_buffer.params.Version_revision != PLAYUAV_VERSION_REVISION) ) { // .. flash the EEPROM with the current actual loaded version eeprom_buffer.params.Version_major = PLAYUAV_VERSION_MAJOR; eeprom_buffer.params.Version_minor = PLAYUAV_VERSION_MINOR; eeprom_buffer.params.Version_revision = PLAYUAV_VERSION_REVISION; bool ret = StoreParams(); if (!ret) { //TODO - handle flash write error here } } }

So, even if the version number stored in EEPROM is briefly wrong, each time the board boots it is corrected. And a boot happens immediately after a firmware flash so it basically will never be wrong.

Having it in the EEPROM is just so we can communicate with the configurator easily, since we already have a mechanism to do that. Even if it were changed or tampered with the change would not stick, and it would never be shown on screen in the OSD.

If you understood all that already, my apologies. Just trying to make sure we understand each other.

TobiasBales commented 8 years ago

1) I think a string is "good enough", we don't need to parse it in c and for js there are plenty options that are easy and fast. Then we don't have to concat, and could also put git shas into there, in case you do a custom build that is not released.

2) The osd respons to a couple commands, one is GET_PARAMS (or something along those lines) which reads the eeprom. There are also other commands like WRITE_TO_EEPROM, READ_CRC etc (again, names are guessed, the code just uses some int constants I think. I would like to add one command GET_FIRMWARE_VERSION (or something like that) which returns that string. The advantage we can get rid of the eeprom restore code and it seems way cleaner to me.

3) Thanks for clearing it up, I understood it that way, the state in the configurator could just be wrong (read osd, flash, don't read again) which would just result in possible inconsistencies (the board then knows the version, the config tool does not, unless params are re-read.

StewLG commented 8 years ago

Ok, hows this for now:

We can get to the version interface later. For now it's most important that we get a user-readable number onscreen.