Allow DFU flashing devices by Device ID #566

JamesHagerman · 2020-04-10T00:55:46Z

Story details: https://app.clubhouse.io/particle/story/51961

Yet another CLI PR I should write tests for!

busticated

some notes otherwise the big blocker is missing tests 👍

busticated · 2020-04-13T15:50:58Z

src/cmd/flash.js

-			.then(() => dfu.findCompatibleDFU())
+			.then(async () => {
+				targetDevice = await dfu.findCompatibleDFU({ deviceId: device });
+			})


nit: i've been holding off on async / await to avoid this king of wackiness. i'd prefer either keeping old-school promises or refactoring the entire method instead of mixing things like this.

busticated · 2020-04-13T16:01:59Z

src/cmd/flash.js

+			if (files.length > 0) {
+				// If both a device and a binary were provided, the binary will come in as the
+				// first element in the files array
+				binary = files[0];


this is ambiguous at best and likely inaccurate. i've been adding tests like this to help clarify similar behavior.

busticated · 2020-04-13T16:03:55Z

src/cmd/flash.js

+
+			// Lookup the Device ID based on the provided Device Name
+			if (device && !isDeviceId(device)) {
+				const api = new ParticleApi(settings.apiUrl, { accessToken: settings.access_token }).api;


at some point i think we're going to have to insist on device IDs across the board as this type of thing leads to confusion - e.g. "why am i getting 4xx errors when flashing locally?!" etc

technobly · 2020-04-13T16:30:20Z

Would be good to add some notes in this PR for how this new feature works. Or a link to a Docs PR.

monkbroc · 2020-04-13T20:38:58Z

The 2 relevant CLI docs pages are:
https://docs.particle.io/reference/developer-tools/cli/
https://docs.particle.io/tutorials/developer-tools/cli/

JamesHagerman · 2020-04-17T18:50:05Z

I've got a lot more work to do on this before it's ready to merge. I'll make this PR a draft until I'm closer to the mark.

busticated

a few more notes - none of which really speak to the elephant in the room 🤗

to that end, i'm thinking we should take a step back and think about how this all could be simplified - e.g. always target by id so we don't need to worry about non -S usage? use particle-usb and related to grab device id? add some special / isolated command namespace to host this functionality? other?

re: testing - extending the e2e suite to expect two devices connected via USB is probably going to be a major pain (mainly b/c other CLI commands don't handle multiple devices well) so as a first step what i'd want to see is solid testing around the happy path (one device) to ensure we aren't breaking existing functionality.

as always, happy to help on both fronts. thanks again for pulling all of this together - sorry for the pain 🙏👍

busticated · 2020-05-08T15:18:12Z

src/cli/keys.js

@@ -73,6 +76,9 @@ module.exports = ({ commandProcessor, root }) => {
 				number: true,
 				description: 'Port number of the server to add to the key'
 			},
+			'device': {
+				description: 'Device ID of the target device',
+			},


we have a number of commands in this same namespace that accept deviceID as a parameter - e.g.

particle-cli/src/cli/keys.js

Lines 56 to 63 in 5659edf

commandProcessor.createCommand(keys, 'doctor', 'Creates and assigns a new key to your device, and uploads it to the cloud', {

params: '<deviceID>',

options: protocolOption,

handler: (args) => {

const KeysCommand = require('../cmd/keys');

return new KeysCommand().keyDoctor(args);

}

});

i think we should either support that call signature (total pain, requires manual arg checking / juggling, etc) - OR - we rename the option (flag) to --deviceID. in both cases we want to be more explicit about the expected input (since device means id or name across the rest of the CLI commands)

busticated · 2020-05-08T15:28:31Z

src/cmd/flash.js

+
+				let deviceInfo;
+				try {
+					deviceInfo = await getDevice({ id: device, api, auth: settings.access_token });


getDevice() from device-util doesn't really do anything special - i'd love it if we could just use the ParticleApi bits directly so eventually i can just remove device-util entirely.

busticated · 2020-05-08T15:40:54Z

src/lib/dfu.js

-					return prompt([question])
-						.then((ans) => {
-							const dfuId = ans.device;
-							module.exports.dfuId = dfuId;


hm. ok. yeah.

(for clarity, i'm referring to our exports usage here not that this line changed 🤗)

Ha! Yeah. Not sure I've ever seen module.exports used quite like that. I guess that's one way to force private members?

In any case, it makes refactoring DFU related code tricky.

JamesHagerman · 2020-06-03T19:55:53Z

Sorry, this is a bit of a rant, but I've been working this over in my head for a bit and need to put my thoughts somewhere...

to that end, i'm thinking we should take a step back and think about how this all could be simplified - e.g. always target by id so we don't need to worry about non -S usage? use particle-usb and related to grab device id? add some special / isolated command namespace to host this functionality? other?

Maintaining backwards compatibility with DFU is difficult. The DFU serial numbers used by older bootloaders don't match the Device ID nor are they even unique between devices. 😞 User intervention is required in cases where more than one older device is in DFU mode at a time.

That said, we do need a better way to host this functionality. If nothing else, we need better separation of concerns between the commands themselves and the DFU implementation. Currently, both the commands AND the command-specific implementations in the DFU module have a good portion of code duplication.

Because of that duplication, I don't think an isolated command namespace would help.

re: testing - extending the e2e suite to expect two devices connected via USB is probably going to be a major pain (mainly b/c other CLI commands don't handle multiple devices well) so as a first step what i'd want to see is solid testing around the happy path (one device) to ensure we aren't breaking existing functionality.

Indeed! While manually testing this, I was just about to commit when I happened to plug in three devices and found new bugs. 🤕

Honestly, DFU isn't going away. While particle-usb might help in some areas there will always be limits on how far we can (or should) re-implement the stuff we've gotta accomplish. So, it's worth treating the DFU layer as a first class citizen at some point.

I can think

In my mind, that means it needs to be restructured. It needs to not lean so heavily on the module.exports hackery for data/state storage. It needs structure to reduce duplication between it's methods. It needs structure for separation of concerns between the calling code and the internal implementation. It needs structure to support testing. And we need tests before we can gracefully restructure it. Chicken, egg, etc.

I've been sitting on this because we have no immediate need for this functionality... but I think this is a really good example of how we might want to tackle other areas of the CLI. My intention is to draw a line in the sand, isolate all refactoring to the lower implementation details while also leaving as much of the existing code in place as possible since that starts quite a yak shave through a bunch of other code. My planned approach has been loosely this:

Attempt to convert dfu.js so it's in the following format:

let something...
module.exports = { something };

If nothing else, just for readability.

But more importantly, that adventure will explicitly call out the module.exports pattern (anti-pattern?) that we're using in each of those methods. Specifically, I'm speaking about code like this, const { listDFUDevices, showDfuModeHelp } = module.exports; which expects module.exports to work in ways I'm not sure is guaranteed in Node.

It would be a good thing to move towards using an Object (or Class) which has a scope we can explicitly manipulate for things like... I don't know, storing data? Writing software? Fixing the world?... as well as moving us towards a way to build test fixtures that don't depend on the internal workings of Node's export implementation for our tests to function.

:huff:

THEN, Export a new object that actually has a manageable scope (a Class, a real Object, whatever) which can sit next to the existing named exports.
Create tests for that new scoped object export
Start moving the USB methods into that object one by one as the tests come up to speed.

That approach should (in theory) let us keep the majority of the calling code as is while we iterate on a refactored implementation/interface. As we go, it will be more apparent where the calling code has to move and whether we need to back track on our implementation to handle unexpected issues.

Obviously, this is all just noise until I sit down and open the editor, but I see a path through the trees!

busticated · 2020-06-04T04:07:45Z

Maintaining backwards compatibility with DFU is difficult

in what sense? can it be rectified by shipping different toolchains? versioning an interface?

it's worth treating the DFU layer as a first class citizen at some point

agree. we simply must be able to write to our devices 100% reliably.

...we need better separation of concerns between the commands themselves and the DFU implementation

absolutely agree. we have this problem throughout this codebase. lots of intermingled stdout.write() calls. etc.

I don't think an isolated command namespace would help

👌

Attempt to convert dfu.js

thanks for describing the refactors - i agree it needs some tending to but at this point you've probably stared at the code longer than i have. my big concern is simply not breaking what we have today... or if we do, that the thing we replace it with is known-good (solid tests). i'll schedule some time for us to chat.

Specifically, I'm speaking about code like this, const { listDFUDevices, showDfuModeHelp } = module.exports; which expects module.exports to work in ways I'm not sure is guaranteed in Node.

with you up until this minor bit. can you elaborate? this is actually i pattern i use a lot and i'm not aware of any unmet expectations..? i was referring to setting the exports object asynchronously - that code for sure breaks in our slowly approaching esm future.

everything else, 👍

Obviously, this is all just noise until I sit down and open the editor, but I see a path through the trees!

nice! that counts 👍

…lity to flash by Device ID when multiple DFU devices are connected.

…ands as they also use DFU under the hood.

JamesHagerman · 2021-07-12T23:26:33Z

Closing this story as this work is stale and is not an often enough requested feature.

JamesHagerman requested review from busticated, avtolstoy and monkbroc April 10, 2020 00:55

busticated suggested changes Apr 13, 2020

View reviewed changes

monkbroc removed request for avtolstoy and monkbroc April 13, 2020 20:37

JamesHagerman force-pushed the ch51961/allow-dfu-flashing-devices-by-device-id branch from d89d2a9 to 4a155ab Compare April 17, 2020 06:03

JamesHagerman marked this pull request as draft April 17, 2020 18:50

JamesHagerman force-pushed the ch51961/allow-dfu-flashing-devices-by-device-id branch from 5ff2f73 to 7416d9f Compare April 22, 2020 19:54

busticated force-pushed the ch51961/allow-dfu-flashing-devices-by-device-id branch from 7416d9f to 8b411d6 Compare May 8, 2020 03:29

busticated reviewed May 8, 2020

View reviewed changes

JamesHagerman added 3 commits August 20, 2020 19:20

Fixing DFU flash when multiple DFU devices are plugged in. Adding abi…

21e703c

…lity to flash by Device ID when multiple DFU devices are connected.

Linting fixes...

ad6a3b7

Adding Device ID as an argument for a number of the related keys comm…

f6b693b

…ands as they also use DFU under the hood.

JamesHagerman force-pushed the ch51961/allow-dfu-flashing-devices-by-device-id branch from 8b411d6 to f6b693b Compare August 21, 2020 02:20

JamesHagerman closed this Jul 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow DFU flashing devices by Device ID #566

Allow DFU flashing devices by Device ID #566

JamesHagerman commented Apr 10, 2020

busticated left a comment

busticated Apr 13, 2020

busticated Apr 13, 2020 •

edited

busticated Apr 13, 2020 •

edited

technobly commented Apr 13, 2020

monkbroc commented Apr 13, 2020

JamesHagerman commented Apr 17, 2020

busticated left a comment

busticated May 8, 2020

busticated May 8, 2020

busticated May 8, 2020

busticated May 8, 2020

JamesHagerman May 19, 2020

JamesHagerman commented Jun 3, 2020

busticated commented Jun 4, 2020

JamesHagerman commented Jul 12, 2021

	commandProcessor.createCommand(keys, 'doctor', 'Creates and assigns a new key to your device, and uploads it to the cloud', {
	params: '<deviceID>',
	options: protocolOption,
	handler: (args) => {
	const KeysCommand = require('../cmd/keys');
	return new KeysCommand().keyDoctor(args);
	}
	});

Allow DFU flashing devices by Device ID #566

Allow DFU flashing devices by Device ID #566

Conversation

JamesHagerman commented Apr 10, 2020

busticated left a comment

Choose a reason for hiding this comment

busticated Apr 13, 2020

Choose a reason for hiding this comment

busticated Apr 13, 2020 • edited

Choose a reason for hiding this comment

busticated Apr 13, 2020 • edited

Choose a reason for hiding this comment

technobly commented Apr 13, 2020

monkbroc commented Apr 13, 2020

JamesHagerman commented Apr 17, 2020

busticated left a comment

Choose a reason for hiding this comment

busticated May 8, 2020

Choose a reason for hiding this comment

busticated May 8, 2020

Choose a reason for hiding this comment

busticated May 8, 2020

Choose a reason for hiding this comment

busticated May 8, 2020

Choose a reason for hiding this comment

JamesHagerman May 19, 2020

Choose a reason for hiding this comment

JamesHagerman commented Jun 3, 2020

busticated commented Jun 4, 2020

JamesHagerman commented Jul 12, 2021

busticated Apr 13, 2020 •

edited

busticated Apr 13, 2020 •

edited