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

Feature/ymodem listen mode #912

Merged
merged 9 commits into from Apr 1, 2016

Conversation

@m-mcgowan
Copy link
Contributor

commented Mar 16, 2016

28800 baud puts the device in listening mode. #865
From there, ymodem can function directly, also it's possible to reset the system into safe listening mode.


  • Review code
  • Test on device
  • Add documentation
  • Add to changelog

@m-mcgowan m-mcgowan added this to the 0.5.x milestone Mar 16, 2016

@technobly

This comment has been minimized.

Copy link
Member

commented on system/inc/system_update.h in 545cf14 Mar 16, 2016

Should this not be explicitly defined to protect against forgetting to update it?

This comment has been minimized.

Copy link
Contributor Author

replied Mar 16, 2016

What do you have in mind? We could make it SYSTEM_FLAG_MAX = SYSTEM_FLAG_STARTUP_SAFE_LISTEN_MODE+1 but when you add a new flag it still needs updating. To my mind the number was obvious and readable.

This comment has been minimized.

Copy link
Member

replied Mar 16, 2016

Since it's an enum, it should always be 1 higher than the last defined element, yeah? And technically they all don't need to be explicitly defined since it looks like we just want to use default values 0-n.

This comment has been minimized.

Copy link
Contributor Author

replied Mar 16, 2016

I've named them explicitly since they are part of the binary interface between the system and application. If someone were to insert an enum in the middle it would change all subsequent enum values, which might go unnoticed. (Although I do have static assertions in the cpp file.) So we could make the enum values implicit so long as they are checked by static asserts. (And consequently we still don't get away from having the enum values explicitly associated with an integer.) I'm not sure what we gain by making them implicit.

@m-mcgowan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2016

The test here is to use ymodem.py or the particle CLI to update firmware via serial.

stty -f /dev/tty.usbXXX 28800 will put the device in listening mode rather than ymodem mode (so LED is blue rather than magenta) but ymodem functions as before. The client can start a ymodem transfer immediately or send the f command from listening mode.

@monkbroc

This comment has been minimized.

Copy link
Member

commented Mar 18, 2016

I'm not familiar with how system flags work but it doesn't look right that a bunch of special cases get added in system_update.cpp. Is this to be improved later?
https://github.com/spark/firmware/pull/912/files#diff-e4a74a9fe08bb33770aceac34b157e46R75
https://github.com/spark/firmware/pull/912/files#diff-e4a74a9fe08bb33770aceac34b157e46R101

@m-mcgowan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2016

The system flags as they were are just booleans, but we can add change handler support to implement backing storage, such as writing a value to DCT or backup registers. That's what the code does. It's special cases, but only because we don't need the overhead of a generic registration mechanism - the behavior is intrinsic to the meaning of the flag.

@m-mcgowan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2016

While waiting for the modem to connect, I connected serial at 28.8k and the device hung. I don't believe the modem hangups that we've seen are a result of #863 but of some general instability in the modem that we are only now seeing with more advanced functionality. (The 28.8k line interrupt is present in factory firmware, so that could be tested also.)

@m-mcgowan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2016

The CLI crash happens with factory electron firmware too. I believe this is unrelated to this PR. As a test, you can use the Device Updater to flash firmware.

@monkbroc monkbroc force-pushed the feature/ymodem_listen_mode branch from 9fb20c8 to 6a1eb08 Mar 22, 2016

@technobly

This comment has been minimized.

Copy link
Member

commented Apr 1, 2016

  • Tested with particle flash --serial tinker.bin
  • Tested with the old python script that the CLI implementation was based on python ymodemscript.py /dev/cu.usbmodem1451 tinker.bin
  • Tested with this python script which does make the LED blink blue (Listening Mode) ...after which I can open a screen session and interact with Listening Mode (i, v, s, m, x) and these commands work.
import serial;
ser = serial.Serial(port='/dev/tty.usbmodem1451', baudrate=28800);
ser.close();
  • Tested all kinds of fancy series of entering Listening Mode, exiting Listening Mode, entering DFU mode, exiting DFU mode, mulitple times, all from the command line... sweeeet.

@technobly technobly merged commit 845a5b4 into develop Apr 1, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@m-mcgowan m-mcgowan deleted the feature/ymodem_listen_mode branch Sep 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.