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

[RSDK-8932] ota docs, local server #361

Merged
merged 15 commits into from
Dec 19, 2024

Conversation

mattjperez
Copy link
Member

@mattjperez mattjperez commented Dec 10, 2024

This PR does three things

  1. adds basic documentation about the OTA feature
  2. makes the micro-rdk-server build use OTA by default
  3. an http2 static file server for development and testing

@mattjperez mattjperez requested a review from a team as a code owner December 10, 2024 20:41
@mattjperez mattjperez changed the title [RSDK-8932] experimental ota docs [RSDK-8932] ota docs, local server Dec 10, 2024
Makefile Show resolved Hide resolved
Cargo.toml Outdated
@@ -8,6 +8,7 @@ members = [
"micro-rdk-server",
"micro-rdk-ffi",
"examples/modular-drivers",
"etc/ota-server",
Copy link
Member

Choose a reason for hiding this comment

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

Let's call this "ota-dev-server" or "ota-test-server" to clarify that it isn't meant for production use.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, forgot about this one. Please remember to rename this before merge.

Copy link
Member Author

Choose a reason for hiding this comment

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

yup, making those code changes now

OTA.md Outdated

## Workflow

In app.viam, add the following to the `services` array; you can alternatively add a `generic` service then edit it to match the following
Copy link
Member

Choose a reason for hiding this comment

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

missing .com, here and at least one other place.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

OTA.md Outdated
Comment on lines 20 to 21
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Indentation looks off here

Copy link
Member Author

@mattjperez mattjperez Dec 19, 2024

Choose a reason for hiding this comment

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

that was more annoying than expected. different editors displayed for whitespace differently and had different space/tab behavior. ended up just using github editor for it

OTA.md Outdated

## Requirements

- An esp32 wrover-e with 8MB of flash memory.
Copy link
Member

Choose a reason for hiding this comment

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

  • "or more"
  • I think WROVER-E as it is usually capitalized when I see it.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

OTA.md Show resolved Hide resolved
OTA.md Outdated

### OTA Build
The `ota` build consists of *only*:
- the type-specific partition header, `esp_app_desc_t`
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 file get used for? Does it need to be kept? Hosted?

Copy link
Member Author

Choose a reason for hiding this comment

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

added note that the output of this is what should be hosted, the download target of an ota update

rust-version.workspace = true

[dependencies]
axum = {version = "0.7.9", features = ["http2"]}
Copy link
Member

Choose a reason for hiding this comment

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

New dependencies here should go up in the root Cargo.toml, sorted in, then pulled in via .workspace=true down here.

Copy link
Member

Choose a reason for hiding this comment

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

Ping on this? I think this and the json formatting are the only things left before merge.

etc/ota-server/src/main.rs Outdated Show resolved Hide resolved
etc/ota-server/src/main.rs Outdated Show resolved Hide resolved
Makefile Outdated
build-esp32-bin:
cargo +esp espflash save-image --package micro-rdk-server --merge --chip esp32 target/xtensa-esp32-espidf/micro-rdk-server-esp32.bin -T micro-rdk-server/esp32/partitions.csv -s 4mb --bin micro-rdk-server-esp32 --target=xtensa-esp32-espidf -Zbuild-std=std,panic_abort --release
build-esp32-bin: build-esp32-ota
cargo +esp espflash save-image --package micro-rdk-server --features=ota --merge --chip esp32 target/xtensa-esp32-espidf/micro-rdk-server-esp32.bin -T micro-rdk-server/esp32/ota_8mb_partitions.csv -s 8mb --bin micro-rdk-server-esp32 --target=xtensa-esp32-espidf -Zbuild-std=std,panic_abort --release
Copy link
Member

Choose a reason for hiding this comment

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

These are getting really quite long, and there are some subtle differences between the two commands that are obscured by other minor variations. Could you please:

  • Go multi-line for these
  • Ensure the flag order between build-esp32-bin and build-esp32-ota is the same, and use the same form of each flag (-T vs --partition-table), and convert any short-form arguments (e.g. -s) into long form?
  • While you are here, could you add --skip-update-check as well?
build-esp32-ota:
        cargo +esp espflash save-image \
            --package micro-rdk-server \
            ...
            --partition-table micro-rdk-server/esp32/ota_8mb_partitions.csv \
            --flash-size 8mb

etc.

Copy link
Member

@acmorrow acmorrow left a comment

Choose a reason for hiding this comment

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

Some new comments and notes on updates. Also looks like a few original comments not addressed quite yet.

build-esp32-bin:
cargo +esp espflash save-image --package micro-rdk-server --merge --chip esp32 target/xtensa-esp32-espidf/micro-rdk-server-esp32.bin -T micro-rdk-server/esp32/partitions.csv -s 4mb --bin micro-rdk-server-esp32 --target=xtensa-esp32-espidf -Zbuild-std=std,panic_abort --release
build-esp32-bin: build-esp32-ota
cargo +esp espflash save-image \
Copy link
Member

Choose a reason for hiding this comment

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

These are looking much better. A few small things:

  • Order of --bin and --partition-table args is not consistent between the two lists.
  • Let's move any "only in one list" flags/args to the end of each. So for build-esp32-bin you already have --merge there, but --flash-size is in the middle.
  • Let's make the actual target file always the last argument.

So the order of the 'sections' is:

  • identical
  • common flag but different args (e.g. partition table)
  • unique (e.g. --merge and --flash-size)
  • file target

Copy link
Member Author

Choose a reason for hiding this comment

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

removed --flash-size as it doesn't appear to be necessary, both make build-* still work.

Copy link
Member Author

Choose a reason for hiding this comment

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

put it back, turns out it did influence the final build size for some reason. It appears like it may be adding padding with --flash-size for the merged binary to be the same size as the flash target. building without is only slightly larger than the ota app image by itself.

OTA.md Outdated Show resolved Hide resolved
OTA.md Outdated

## Full Build

If a device is built with the above partition table, the `make build-esp32-bin` command creates a Merged Binary that includes
Copy link
Member

Choose a reason for hiding this comment

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

Above you say "Merged Image" but here "Merged Binary".

OTA.md Outdated
- the bootloader
- the partition table mapping (`partitions.csv`)
- populated partitions (with partition headers) according to the mapping
/
Copy link
Member

Choose a reason for hiding this comment

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

Stray /?

OTA.md Outdated
You can confirm this by using `ls -l** in your build directory to compare the size of the binary to your partition table.

## OTA Build
The `ota` build produces an app image (described above), which internally consists of:
Copy link
Member

Choose a reason for hiding this comment

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

In the ## Full Build section above, you describe make build-esp32bin early, as the task to be performed, but this section doesn't have that. I thihnk it should

OTA.md Outdated Show resolved Hide resolved
etc/ota-server/src/main.rs Outdated Show resolved Hide resolved
Copy link
Member

@acmorrow acmorrow left a comment

Choose a reason for hiding this comment

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

LGTM mod renaming ota-server and hoisting the new crate dependencies up into the root Cargo.toml.

@mattjperez
Copy link
Member Author

Copy link
Member

@gvaradarajan gvaradarajan left a comment

Choose a reason for hiding this comment

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

LGTM, optional suggestions

OTA.md Outdated

The `version` field is equivalent to a `tag` and can be any arbitrary string of up to 128 characters.
After successfully applying the new firmware, this `version` will be stored in NVS.
This values is compared to that of the latest machine config from app.viam.com and will trigger the update process.
Copy link
Member

Choose a reason for hiding this comment

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

values -> value? Also maybe clarify that a detected change in the version is what prompts the update process

OTA.md Outdated
- for example, make a device capable of OTA.

**This is not the build that should be hosted at the `url` in the service config.**
You can confirm this by using `ls -l** in your build directory to compare the size of the binary to your partition table.
Copy link
Member

Choose a reason for hiding this comment

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

should we clarify this with the expected size difference?

OTA.md Outdated

While not all blob storage platform support HTTP/2, many offer Content Delivery Network (CDN) solutions that do.

We don't currently support tokens in the [OTA Service Config](#ota-service-config), so if permissions are required to access the endpoint they must be embedded in the URL itself.
Copy link
Member

Choose a reason for hiding this comment

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

tokens -> authentication tokens. embedded in the URL itself -> embedded in the URL as query params?

@mattjperez mattjperez merged commit 7038313 into viamrobotics:main Dec 19, 2024
6 checks passed
@mattjperez mattjperez deleted the rsdk-8932-ota-docs branch December 19, 2024 19:24
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.

3 participants