Skip to content

Conversation

@microbit-matt-hillsdon
Copy link
Contributor

@microbit-matt-hillsdon microbit-matt-hillsdon commented Oct 2, 2022

Provide a minimal implementation that allows reading the device ID but otherwise raises an exception.

I'm open to adding other FICR but this is the priority to get the code on this support article working in the simulator.

Closes #58

@netlify
Copy link

netlify bot commented Oct 2, 2022

Deploy Preview for distracted-dubinsky-fd8a42 ready!

Name Link
🔨 Latest commit c042b3c
🔍 Latest deploy log https://app.netlify.com/sites/distracted-dubinsky-fd8a42/deploys/6339b007ef7d9b000871eb14
😎 Deploy Preview https://deploy-preview-78--distracted-dubinsky-fd8a42.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@microbit-matt-hillsdon microbit-matt-hillsdon force-pushed the machine-mem branch 2 times, most recently from 395eaf6 to 841b12c Compare October 2, 2022 15:35
Provide a minimal implementation that allows reading the device ID but
otherwise raises an exception.
@microbit-matt-hillsdon
Copy link
Contributor Author

@microbit-carlos this PR fails all writes and all reads other than the device ID (which we push folks towards in our support article, for better or worse). Queries:

  1. Are there other FICR values that are likely to be used?
  2. Do you think raising an exception is reasonable? I wouldn't expect many folks to correctly interpret DEADCODE or similar and it would be good to find out if there are gaps that we could reasonably simulate rather than just have code fail silently.

@microbit-matt-hillsdon
Copy link
Contributor Author

Still interested in the responses to the above but I think I'm happy to merge this once Rob has had a chance to review. It's certainly an improvement on the current behaviour.

Copy link
Contributor

@microbit-robert microbit-robert left a comment

Choose a reason for hiding this comment

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

LGTM. Referenced code in support article runs as expected.

@microbit-matt-hillsdon microbit-matt-hillsdon merged commit b721da6 into main Oct 4, 2022
@microbit-matt-hillsdon microbit-matt-hillsdon deleted the machine-mem branch October 4, 2022 16:08
@microbit-carlos
Copy link
Collaborator

  1. Are there other FICR values that are likely to be used?

Can't think of others are likely to be used, but there a few that are simple to replicate in the sim, mostly page size (4K), flash and RAM size, and the nordic part type (52833). But yeah, not worth implementing it unless we have a real use case (I've only used them for development and testing) or users request it.

  1. Do you think raising an exception is reasonable? I wouldn't expect many folks to correctly interpret DEADCODE or similar and it would be good to find out if there are gaps that we could reasonably simulate rather than just have code fail silently.

My first instinct was to return a "known incorrect" value, so that the sim wouldn't throw an exception that the device wouldn't throw, but to be fair if they are doing anything with the value, they'd have to either do an extra check for the sim value, or wrap it in a try/except for the sim as well, so as both cases need sim-specific code, I think it's fine as it is 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

machine.mem32 etc. need a HAL abstraction

4 participants