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.

Sign up for GitHub

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

Add device.delete #30

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add device.delete #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/dusb/magic-values.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it came from libticalcs. I'm not sure why it's separate from the other type attribute in the first place.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TI logic ? :)

},

// Transfer modes
Expand Down
17 changes: 17 additions & 0 deletions src/dusb/ti84series.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,23 @@ module.exports = class Ti84series {
flash: params.find(p => p.type == v.parameters.DUSB_PID_FREE_FLASH).value,
};
}

async delete(name, type) {
await this._d.send({
type: v.virtualPacketTypes.DUSB_VPKT_MODIF_VAR,
data: [
0, name.length,
...b.asciiToBytes(name),
...b.constructParameters([{
type: v.attributes.DUSB_AID_VAR_TYPE2,
value: b.intToBytes(0xF0070000 + type, 4)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No bounds check on type. Sure, other parts are already doing it wrong too, but no need to introduce more low-quality code :)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That, and also that different models have different types available in the first place.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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..? :)

}]),
1,
0, 0, 0, 0, 0
]
});
await this._d.expect(v.virtualPacketTypes.DUSB_VPKT_DATA_ACK);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like something for a separate PR.

}

// Send a TI file to the calculator
async sendFile(file) {
Expand Down