Add device.delete #30
Conversation
| ...b.asciiToBytes(name), | ||
| ...b.constructParameters([{ | ||
| type: v.attributes.DUSB_AID_VAR_TYPE2, | ||
| value: b.intToBytes(0xF0070000 + type, 4) |
There was a problem hiding this comment.
No bounds check on type. Sure, other parts are already doing it wrong too, but no need to introduce more low-quality code :)
There was a problem hiding this comment.
I'm not sure that type should be a user-supplied value in the first place. How am I, as a user of ticalc-usb, supposed to know the possible values for this parameter? :) I'd prefer if we could abstract this knowledge away.
Ideas:
- Auto-detect the type (do a
getDirectory, then find by name). With the downside that we may delete the wrong thing, or error when there are two options. - Supply a map of types for the user to choose from (
ticalc.variableTypes.program) that we can also check against - Let the user supply a string from a set of valid strings (
'program')
There was a problem hiding this comment.
The type that this accepts is the same that getDirectory returns, and the same value as is stored in the entry. Automatic type detection is in my opinion a bad idea, since it might result in code that seems to behave most of the time but then does something completely different the rest of the time. I think an enumeration of types would be the best solution overall.
There was a problem hiding this comment.
An enumeration of types would work, but it would be a large enum, since types are model-dependent. libtifiles splits types across multiple types*.h files.
There was a problem hiding this comment.
Really? Why is that? I assume because something like program equates to different numeric values depending on the model? If that's the case then again we can solve that issue from the perspective of the API user, because we know the model and we can do the mapping in the library.
There was a problem hiding this comment.
That, and also that different models have different types available in the first place.
There was a problem hiding this comment.
Check. Then I'd expect the API to allow the superset of types, convert the right ones and give an error if we try to use a type that isn't valid for the current model. Right..? :)
| 0, 0, 0, 0, 0 | ||
| ] | ||
| }); | ||
| await this._d.expect(v.virtualPacketTypes.DUSB_VPKT_DATA_ACK); |
There was a problem hiding this comment.
AFAICT, this, or expect() in device, does not handle error packets properly. See dusb_cmd_r_data_ack() in https://github.com/debrouxl/tilibs/blob/experimental2/libticalcs/trunk/src/dusb_cmd.cc .
Again, other parts of the code are already doing it wrong, but in order to enable proper long-term maintainability and growth of this code base, removing some of the excessive corner-cutting while adding other features is a good thing :)
There was a problem hiding this comment.
Sounds like something for a separate PR.
| @@ -92,6 +92,7 @@ module.exports = { | |||
| DUSB_AID_VAR_TYPE: 0x02, | |||
| DUSB_AID_ARCHIVED: 0x03, | |||
| DUSB_AID_VAR_VERSION: 0x08, | |||
| DUSB_AID_VAR_TYPE2: 0x11, | |||
There was a problem hiding this comment.
Any idea what this is or does? I'm guessing this name came from libticalc? Maybe we can name it something a bit more descriptive, or add a comment.
There was a problem hiding this comment.
Yes, it came from libticalcs. I'm not sure why it's separate from the other type attribute in the first place.
| 0, 0, 0, 0, 0 | ||
| ] | ||
| }); | ||
| await this._d.expect(v.virtualPacketTypes.DUSB_VPKT_DATA_ACK); |
There was a problem hiding this comment.
Sounds like something for a separate PR.
| ...b.asciiToBytes(name), | ||
| ...b.constructParameters([{ | ||
| type: v.attributes.DUSB_AID_VAR_TYPE2, | ||
| value: b.intToBytes(0xF0070000 + type, 4) |
There was a problem hiding this comment.
I'm not sure that type should be a user-supplied value in the first place. How am I, as a user of ticalc-usb, supposed to know the possible values for this parameter? :) I'd prefer if we could abstract this knowledge away.
Ideas:
- Auto-detect the type (do a
getDirectory, then find by name). With the downside that we may delete the wrong thing, or error when there are two options. - Supply a map of types for the user to choose from (
ticalc.variableTypes.program) that we can also check against - Let the user supply a string from a set of valid strings (
'program')
Add a method for deleting a variable.