Bootloader issues

[vidya88]

@YuraYelisieiev commented on Jul 18

In general:

• We need to get rid of the static asserts in files. For example in storage.c. (no, see comments below )

rng.h

• We need to change the include structure. Because now the way it compiles is not appropriate.

signatures.c:

• why do need SIG_OK, SIG_FAIL for signatures ok, if it just can return true or false
• writing the same code thrice isnt the best way. I think that better write a cycle.

bootloader.c:

• Here button is checked after 100000 cycles of nop, while some of button is pressed. What is the sense of it, because user can quick press in such way, that bootloader won't see it. And in this time processor is literally doing nothing, so it is better to all this time just checking, if button was pressed

firmware_sign_split.py:

what is the sense of this file, if it this never used ?

usb.c:

• Here is checking if new firmware is valid and then,if not, wiped from skywallet, but instead just wiped backup of meta, which is not important, because never used(in the next line it is replaced by new meta.
• Also multiple times are repeating lines from here to here, so would be good to make function from them.
• Test of the functionality HERE.

1. Are you confident that this number is good enough to test the uniform distribution? Link
2. CPU status is not implemented yet Link . Is it ok?
3. Can you give me a detailed description of the USB test HERE?

Created by @RomanMilishchuk and @YuraYelisieiev

[vidya88]

@jdknives commented on Jul 19

Thanks for the review. Do I understand that correctly that questions 1-3 need to be answered for v1 and the bullet points above are improvement proposals or questions for v2? @RomanMilishchuk @YuraYelisieiev

[vidya88]

@gz-c commented on Jul 19

What is wrong with the _Static_assert?

[vidya88]

@olemis commented on Jul 21

@gz-c commented on Jul 19

What is wrong with the _Static_assert?

There is nothing wrong with them . They are there to ensure some preconditions are met at compile time . Removing those can lead to major issues.

[vidya88]

@YuraYelisieiev commented on Jul 22

@olemis commented on Jul 21

@gz-c commented on Jul 19 What is wrong with the _Static_assert?

There is nothing wrong with them . They are there to ensure some preconditions are met at compile time . Removing those can lead to major issues.

As I remember Asserts needs to be removed in the release because developers uses them only during the testing period.

[vidya88]

@gz-c commented on Jul 22

these asserts should not be stripped out of releases. if any of these asserts fail the program should abort immediately

[vidya88]

@RomanMilishchuk commented on Jul 22

@jdknives As for me, next problems should fixed for v1:

• Here button is checked after 100000 cycles of nop, while some of button is pressed. What is the sense of it, because user can quick press in such way, that bootloader won't see it. And in this time processor is literally doing nothing, so it is better to all this time just checking, if button was pressed
• Here is checking if new firmware is valid and then,if not, wiped from skywallet, but instead just wiped backup of meta, which is not important, because never used(in the next line it is replaced by new meta.

• And the questions 1 -3

[vidya88]

@olemis commented on Jul 25

@gz-c commented on Jul 22

these asserts should not be stripped out of releases. if any of these asserts fail the program should abort immediately

indeed , if assertions are not met code will fail to be compiled .

[vidya88]

@RomanMilishchuk commented on Jul 25

@olemis this was written above and canceled a few days ago, could you reply to my last comment above?

[vidya88]

@stdevAlDen commented on Jul 26

@YuraYelisieiev commented on Jul 18

In general:

• ~We need to get rid of the static asserts in files. For example in storage.c.~ (no, see comments below )

rng.h

• We need to change the include structure. Because now the way it compiles is not appropriate.

related to this? signatures.c:

• why do need SIG_OK, SIG_FAIL for signatures ok, if it just can return true or false there is no bool data type in c99, no true no false, just integers.
• writing the same code thrice isnt the best way. I think that better write a cycle.

please use a permanent link here bootloader.c:

• Here button is checked after 100000 cycles of nop, while some of button is pressed. What is the sense of it, because user can quick press in such way, that bootloader won't see it. And in this time processor is literally doing nothing, so it is better to all this time just checking, if button was pressed

firmware_sign_split.py:

what is the sense of this file, if it this never used ?

maybe related to this #27 usb.c:

• Here is checking if new firmware is valid and then,if not, wiped from skywallet, but instead just wiped backup of meta, which is not important, because never used(in the next line it is replaced by new meta.
• Also multiple times are repeating lines from here to here, so would be good to make function from them.
• Test of the functionality HERE.

1. Are you confident that this number is good enough to test the uniform distribution? Link
2. CPU status is not implemented yet Link . Is it ok?
3. Can you give me a detailed description of the USB test HERE?

maybe this is unused, I can not find it from here. Created by @RomanMilishchuk and @YuraYelisieiev @YuraYelisieiev & @RomanMilishchuk pls check out comments.

[vidya88]

@jdknives commented on Jul 26

@stdevAlDen thanks for the comments. However it seems that the most important part, the questions 1-3 at the end were not answered. Would you mind doing that?

[vidya88]

@stdevAlDen commented on Jul 26

@YuraYelisieiev commented on Jul 18

In general:

• ~We need to get rid of the static asserts in files. For example in storage.c.~ (no, see comments below )

rng.h

• We need to change the include structure. Because now the way it compiles is not appropriate.

signatures.c:

• why do need SIG_OK, SIG_FAIL for signatures ok, if it just can return true or false
• writing the same code thrice isnt the best way. I think that better write a cycle.

bootloader.c:

• Here button is checked after 100000 cycles of nop, while some of button is pressed. What is the sense of it, because user can quick press in such way, that bootloader won't see it. And in this time processor is literally doing nothing, so it is better to all this time just checking, if button was pressed

firmware_sign_split.py:

what is the sense of this file, if it this never used ?

usb.c:

• Here is checking if new firmware is valid and then,if not, wiped from skywallet, but instead just wiped backup of meta, which is not important, because never used(in the next line it is replaced by new meta.
• Also multiple times are repeating lines from here to here, so would be good to make function from them.
• Test of the functionality HERE.

1. Are you confident that this number is good enough to test the uniform distribution? Link
2. CPU status is not implemented yet Link . Is it ok?
3. Can you give me a detailed description of the USB test HERE?

@jdknives sure. I have no idea on how much this matter for the bootloader, but I found an important detail, this code does not exist in the original project. In addition consider my last comment here. PD: investing more time I can come back with a better answer/solution but I'm fighting with this right now, please let me know if this have priority. Created by @RomanMilishchuk and @YuraYelisieiev

[vidya88]

@olemis commented on Jul 30

in any case this is not a v1 issue