Update docs #87

utzig · 2017-07-20T12:26:13Z

No description provided.

Signed-off-by: Fabio Utzig <utzig@apache.org>

mbolivar · 2017-07-20T16:03:12Z

doc/design.txt

    ~                                                               ~
    ~             Swap status (128 * min-write-size * 3)            ~
    ~                                                               ~
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
-    |   Copy done   |     0xff padding (up to min-write-sz - 1)     ~
+    |           0xff padding (7 octets)             |   Copy done   |


Shouldn't this be:

| Copy done | 0xff padding (7 octets) |

Similarly for image OK.

Signed-off-by: Fabio Utzig <utzig@apache.org>

mbolivar · 2017-07-20T16:04:21Z

doc/design.txt

@@ -24,7 +24,7 @@
 The Mynewt bootloader comprises two packages:


If you're updating this to note that there are multiple ports, how about saying "mcuboot" instead of "The Mynewt bootloader"?

mbolivar · 2017-07-20T16:08:18Z

README.md

@@ -10,16 +10,13 @@ easy software upgrade.
 MCUboot is operating system and hardware independent, and relies on 
 hardware porting layers from the operating system it works with.  Currently
 mcuboot works with both the Apache Mynewt, and Zephyr operating systems, but
-more ports are planned in the future.
+more ports are planned in the future. RIOT is currently supported as a boot
+target with complete port arriving in the near future.


"a complete port planned", maybe?

mbolivar · 2017-07-20T16:11:17Z

doc/design.txt

@@ -210,15 +210,15 @@ An image trailer has the following structure:
     0                   1                   2                   3


There is nothing wrong with this line, but the GitHub UI won't let me comment on parts of the file that aren't in a diff hunk.

It looks like the "BOOT STATES" section above also needs to be reworked in light of the recent changes as well.

I don't think it does. I know what you mean, but there are two ways of explaining things in the design doc: one more "abstract", which uses words like pending and confirmed and the practical, real implementation, which uses magic, image_ok and copy_done.

If pending is one of "unset", "temporary", "permanent", and confirmed is one of "set", "unset", then with two slots there are 36 possible initial abstract "BOOT STATES", but only 4 are documented above.

Does that mean that the above text "the four possible states that the device can be in" really means something along the lines of "the four permitted states that mcuboot recognizes as valid boot states"?

Will the other 32 states reliably trigger a "panic" swap type in the new update code?

Edit: and what about the fact that "pending" can actually be "bad", in the sense that an image is "pending" when its magic is set, but its magic can be unset, set, or "bad"?

You're doing permutation, while it would be more correct to consider it like a tree of states, if magic is unset do nothing, if magic is set check the other flag and so on and so forth.

Will the other 32 states reliably trigger a "panic" swap type in the new update code?

Nope, a weird state should either be NONE or FAIL which end up with a somewhat similar behavior.

You're doing permutation, while it would be more correct to consider it like a tree of states, if magic is unset do nothing, if magic is set check the other flag and so on and so forth.

I disagree with this assessment.

I'm enumerating all of the states and asking what the boot outcome would be in each one, which is a perfectly correct thing to do when considering "possible" states.

A decision tree representation is equivalent to a complete enumeration, which can be more compactly represented if many outcomes are equivalent due to some components being "don't care", such as your example of "if magic is unset do nothing".

Will the other 32 states reliably trigger a "panic" swap type in the new update code?

Nope, a weird state should either be NONE or FAIL which end up with a somewhat similar behavior.

Statements like this worry me.

If this is the design document for how the bootloader is supposed to make decisions, and we're still saying that states not listed here (despite the document claiming this is all "possible" states) "should" result in an outcome which is "either [...] NONE or FAIL", which is "somewhat similar behavior", it means that the design has not been completely thought through.

I think I agree with you. I'll give it a try on mapping this in a less loose way...

I'm enumerating all of the states and asking what the boot outcome would be in each one, which is a perfectly correct thing to do when considering "possible" states.

A decision tree representation is equivalent to a complete enumeration

Further, the document above is not described as a decision tree, but rather an enumeration of states. Perhaps if that's what the code is really doing, then reworking the "BOOT STATES" section so it's described a decision tree is what is needed? Then we can compare that to how the concrete "vector states" (with their "Any" values) map to that tree, to convince ourselves things work the way we mean them to.

mbolivar · 2017-07-20T16:14:51Z

doc/design.txt

@@ -272,7 +272,7 @@ purposes; they are not actually present in the boot vector.
    -----------------+--------+--------'
     swap: none                        |
    -----------------------------------'
-


Sorry, I'm confused about this (and actually always have been).

This section says there are 5 states. The "BOOT STATES" section above says there are only 4. The code in boot_swap_tables now actually only has 3 entries.

What's going on here?

There are 3 states that need special handling: test, permanent and revert and those are in the tables. There is one additional state which does nothing: none. It just ends up with the image in slot 0 booting. This last one never gets to the swapping routines which employ the swap table you mentioned.

Something is still fishy here. In the paragraph just above this, it says "It is better to map all the possible vector states to the three states described above via a set of tables".

However:

As discussed above, this is not "all possible" vector states

3 != 4 (and there are four states listed in the BOOT STATES section)

Further, "State I" and "State V" here both lead to the "none" swap type, which is two "states", not "one additional state" as you wrote in the above comment.

I still think that this document is not consistent with itself as a result; what am I missing?

3 != 4 (and there are four states listed in the BOOT STATES section)

I agree with this and I think the sentence should be ""It is better to map all the possible vector states to the four states described...".

Further, "State I" and "State V" here both lead to the "none" swap type, which is two "states", not "one additional state" as you wrote in the above comment.

Right, "State I" and "State V" are really two states in the flash. But both of them end up being NONE in the code.

Right, "State I" and "State V" are really two states in the flash. But both of them end up being NONE in the code.

Well, they're two "vector states", which both lead to the same "swap state" or "swap result" ("vector state" being an input to the bootloader, "swap state" being an output or at least an intermediate output).

You bring up a good point in the earlier sections discussion that this isn't really a listing of states so much as it is a sequential listing of what the bootloader is checking. Perhaps that is a better way to write this section too?

Signed-off-by: Fabio Utzig <utzig@apache.org>

mbolivar · 2017-07-20T17:55:20Z

doc/design.txt

    -----------------+--------+--------'
             pending |        |        |
          confirmed  |        |   X    |
    -----------------+--------+--------'
     swap: revert (test image running) |
    -----------------------------------'

+Note: copy-done is only used to determine if the current image was the result
+of a swap operation or if it was written by the provided imgtool, in which case
+no revert is performed. All other states don't need it.


How about using "Any" lines to show that instead?

Signed-off-by: Fabio Utzig <utzig@apache.org>

d3zd3z · 2017-07-20T18:21:19Z

README.md

-http://lists.runtime.co/mailman/listinfo/dev-mcuboot_lists.runtime.co
+* Our developer mailing list:
+  http://lists.runtime.co/mailman/listinfo/dev-mcuboot_lists.runtime.co
+* Our Slack channel: https://runtimeco.slack.com/


I believe Slack requires an invitation to join. Not sure if it would be obvious to people that they need to send email and ask for an invite.

d3zd3z · 2017-07-20T18:22:36Z

doc/design.txt

 confirmed as good by the user (0x01=confirmed; 0xff=not confirmed).

+4. MAGIC: The following 16 bytes, written in host-byte-order:


host-byte-order? Shouldn't it be written in target byte order, since that what will be checking it?

Ah, I haven't seen that but it confuses me anyway. host-byte-order, target-byte-order, network-byte-order, etc all seem bad. I think it should be either little-endian or big-endian.

d3zd3z · 2017-07-20T18:24:35Z

release-notes.txt

@@ -0,0 +1,19 @@
+mcuboot 0.9 - Release Notes


Do we want to put this document in either markdown or RST. This formatting looks like RST, and naming it that would make it format nicely when viewed in github.

Signed-off-by: Fabio Utzig <utzig@apache.org>

utzig · 2017-07-20T20:53:03Z

Design doc will be updated before official release. Tracked by issue: https://runtimeco.atlassian.net/browse/MCUB-67

utzig added 4 commits July 20, 2017 08:55

Update design document to match implementation

5bd4e58

Signed-off-by: Fabio Utzig <utzig@apache.org>

Apply a few updates to README

097f76b

Signed-off-by: Fabio Utzig <utzig@apache.org>

Move release notes to a more common format

485f837

Signed-off-by: Fabio Utzig <utzig@apache.org>

Add IRC channel information

7402374

Signed-off-by: Fabio Utzig <utzig@apache.org>

utzig requested review from mbolivar and d3zd3z July 20, 2017 12:31

mbolivar reviewed Jul 20, 2017

View reviewed changes

Fix trailer's status flags ordering

2c305aa

Signed-off-by: Fabio Utzig <utzig@apache.org>

mbolivar suggested changes Jul 20, 2017

View reviewed changes

utzig added 3 commits July 20, 2017 13:20

Change references of Mynewt to mcuboot

ac83496

Signed-off-by: Fabio Utzig <utzig@apache.org>

Fix sentence wording per review

756ca86

Signed-off-by: Fabio Utzig <utzig@apache.org>

Add slack channel

dead387

Signed-off-by: Fabio Utzig <utzig@apache.org>

mbolivar reviewed Jul 20, 2017

View reviewed changes

Add 'Any' states for copy-done

f9d4428

Signed-off-by: Fabio Utzig <utzig@apache.org>

d3zd3z reviewed Jul 20, 2017

View reviewed changes

Rename file

095757c

Signed-off-by: Fabio Utzig <utzig@apache.org>

utzig merged commit 253bf0f into mcu-tools:master Jul 20, 2017

utzig deleted the update-docs branch July 26, 2017 10:40

aditihilbert mentioned this pull request Feb 5, 2018

Design document needs to have better definition of boot states behavior #214

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update docs #87

Update docs #87

utzig commented Jul 20, 2017

mbolivar Jul 20, 2017

mbolivar Jul 20, 2017

mbolivar Jul 20, 2017

mbolivar Jul 20, 2017

utzig Jul 20, 2017

mbolivar Jul 20, 2017 •

edited

Loading

utzig Jul 20, 2017

utzig Jul 20, 2017 •

edited

Loading

mbolivar Jul 20, 2017

mbolivar Jul 20, 2017 •

edited

Loading

utzig Jul 20, 2017

mbolivar Jul 20, 2017

mbolivar Jul 20, 2017

utzig Jul 20, 2017

mbolivar Jul 20, 2017 •

edited

Loading

utzig Jul 20, 2017

utzig Jul 20, 2017

mbolivar Jul 20, 2017

mbolivar Jul 20, 2017

utzig Jul 20, 2017

d3zd3z Jul 20, 2017

d3zd3z Jul 20, 2017

utzig Jul 20, 2017

d3zd3z Jul 20, 2017

utzig commented Jul 20, 2017

		@@ -24,7 +24,7 @@
		The Mynewt bootloader comprises two packages:

		@@ -210,15 +210,15 @@ An image trailer has the following structure:
		0 1 2 3

		confirmed as good by the user (0x01=confirmed; 0xff=not confirmed).

		4. MAGIC: The following 16 bytes, written in host-byte-order:

Update docs #87

Update docs #87

Conversation

utzig commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbolivar Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utzig Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbolivar Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbolivar Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utzig commented Jul 20, 2017

mbolivar Jul 20, 2017 •

edited

Loading

utzig Jul 20, 2017 •

edited

Loading

mbolivar Jul 20, 2017 •

edited

Loading

mbolivar Jul 20, 2017 •

edited

Loading