Great review - thanks Peter! Right off the bat I have a couple questions, but may have more as I work on this.
1) Logically I see why Peripheral is the right base class as it's a device on top of other interfaces, but from an interface perspective it aligns with IO better. Peripheral doesn't have read/write (so those are extensions) and instead expects Sample to be used.
2) Breaking up the options on the constructor makes sense - I'll pull out the SPI pins so they can live in device.io.SPI (clock, in, out, ...) which I think was your intent as well. I'll need Heltec_Wifi_Kit_32 defined for the device.io.* object (or perhaps we create a new one for Heltec_Wifi_LoRa_32_V2?). What's the best way to go about that? Here are the Heltec Wifi LoRa 32 V2 pins
3) configure vs. get/set - Set is used in the Serial driver so that's what I adopted as it matched the best. Perhaps Peripheral.configure is the better interface, in which case I'd drop get as I only added it to comply with how Serial did it. If using IO base class, maybe get/set is better and if using Peripheral configure is?
4) read - I'd like to keep pre-allocated buffers for my use case, but can implement read(number) which returns a dynamically allocated buffer up to max(number) bytes.
Update to the above:
2) Ignore my pull-out of SPI, I see that is "lora" in your example. But since that is SPI, where do I put radio configuration settings like bandwidth, codingRate, frequency, etc? Should "lora" be renamed to something like "interface" and then "lora" have configuration settings? Likewise, where does "onReadable"/"onWritable" go - at the root of the object?
size parameter (and on read to always just dynamically allocate). I've always tried to eliminate dynamic allocations as much as possible in embedded systems as it increases risk of fragmentation and failure on long running systems. Are you not concerned about this with XS?
All recommended changes are made, but will further alter based on any feedback from above. Constructor dictionary has sections lora/interrupt/reset as recommended, all other values sit at the root of the dictionary. Now uses configure() instead of set() but still has get(). Close is now safe to call multiple times. Read supports read(buffer) as well as read(number). Write is no longer blocks as is the same for sync/async. typeof undefined changed as recommended. Also extended the library with several new features for the ST127x chip (such as more statistics, gain control, and small packet / high speed chip optimization).
Here is the latest version as well as a crappy test main to run it (uses build parameter "transmit=0" to build a receive node, and "transmit=1" to build a transmit node). Note: If there is a better way to convert strings to/from ArrayBuffer (see main.js) I'm all ears!
serial2xsbug and the output window shows interspersed output from both devices. Switching between tabs cleans up the output to be per-device again.
Changes look good. Just a few quick comments:
- Agreed that Lora is more IO than Peripheral. I think
configuremakes sense here, since that's exactly the behavior you implemented.get/setis specific for Serial and named to match Web Serial, but I can see how that would be unclear from just the spec. - I think
writecall should usesizeinstead ofbuf.byteLengthwhen looping to send bytes to the FIFO. - I got it wrong when suggesting
lorafor the property name. The Microchip digital expander is a good example - eitherspiori2cis specified depending on the interface. Even with only one supported interface here, I think would make sense to follow here too, sospiinstead orlora. - This
undefined !== typeof options.enableReceiverdoesn't avoid the string compare (but make it harder to see!).undefined !== options.enableReceiverdoes.
I'm really looking forward to trying this out.
FYI: Small bug in xsbug when running multiple devices. First machine launches fine in a tab. Second machine starts
serial2xsbugand the output window shows interspersed output from both devices. Switching between tabs cleans up the output to be per-device again.
That's a feature, not a bug, When a machine tab is selected, it shows that tab's output. When nothing is selected, it shows them all (merged in time order). That let's you watch several machines at once.
@bburky Thank you for the M5Paper suggestion above! I can confirm that that looks a lot better when switching between apps on my M5Paper device.
@phoddie and I have been iterating a bit on the tidiest way to implement this idea in the examples. I think we've settled on this:
class AppBehavior extends Behavior {
onDisplaying(application) {
screen.refresh?.();
screen.configure?.({updateMode: config.firstDrawMode ?? config.updateMode});
if (config.firstDrawMode)
application.defer("onFinishedFirstDraw", config.updateMode);
}
onFinishedFirstDraw(application, mode) {
screen.configure({updateMode: mode});
}That allows for setting both firstDrawMode and updateMode per-device in the manifest and also works in the simulator. I'm going to go ahead and make that change in the examples and add a note to our M5Paper documentation.
JSON.stringify('test\n') I get "test\n" from Moddable, but I get "test\\n" (with the newline encoded) from my Chrome browser. I need it to be encoded, so trying to understand if this is expected (and I need to write some encoder that works on the messages) or if it's perhaps a bug?
FYI - small mistake on XS Mods page - says to add Modules to your manifest:
"include": [
$(MODULES)/module/manifest.json
]But it's actually
"include": [
$(MODULES)/base/modules/manifest.json
]replace-function-object-deleted-property test. That isn't definitive -- Test262 doesn't hit everything -- but it is usually a good sign.
onWritable callback support fell out: #writeCompletion is called when the interrupt fires but there's no code to call onWritable. (I'll set up the other board Wednesday to try transmit & receive together).
@phoddie Doh! Fixed ... and I tested callbacks a bit and found several bugs (which required reworking receive). That's what I get for not investing in a test harness (which I still haven't done...!)
I also changed my buffer management - before I was directly access ArrayBuffer, such as:
let s = "my message";
let x = new ArrayBuffer(s.length);
for (let i = 0; i < x.byteLength; ++i)
x[i] = s.charCodeAt(i);However, I found this incompatible with ArrayBuffer.fromString("my messsage") - each element of x[i] would be undefined. That surprised me ... I could understand byte alignment issues or weird data, but undefined doesn't make sense on an array that has data).
To fix it, I added a view on the buffer, using Uint8Array. The code works, but now I have a dynamic allocation on every transmit and receive (new Uint8Array(buf)). Any ideas how to eliminate this dynamic allocation? I think it's the only allocation that occurs (when using static buffers).
onWritable is likely an inappropriate use case for most implementations on LoRa because the normal thing to do is to use it to send another packet, but LoRa FCC (and other country) regulations do not allow that. My main test code violates FCC rules as it sends once/second, exceeding airtime rules.
onWritable could be useful if you wanted to put the MCU into deep sleep for a while to save power. Knowing exactly when the packet has gone out would let the power down happen as soon as possible. But, I'm guessing based on experience with other packet based communication because LoRa is all new to me.
close it puts the radio in sleep, so that is what you would want to do prior to going to sleep to further reduce power.
VERS) and Linux has 0x0b05 (in the xsa file). I assume that means my Linux system is running a more recent version of Moddable (likely, as it runs in an auto-generated Docker container so currently stays quite fresh). Is it accurate that base firmware and mods must run the exact same version of Moddable?
ArrayBuffer is pre-allocated at 250 bytes. The data arrives and I use String.fromArrayBuffer(buffer)to get the string and print it. ArrayBuffer.byteLength will still be 250 bytes, and as long as the buffer is fresh it works great - but variable size messages will cause this issue. What's best practice here - I see from your book they default to zeros (like calloc), so should I add a trailing null byte to the buffer in the lora code (assuming space available in the buffer) for safety? Or is this just left to the caller to deal with, which if using String.fromArrayBuffer(buffer) likely means an extra dynamic allocation to use something like String.fromArrayBuffer(buffer.slice(0, buffer.byteLength))?
I think it's easy to explain (come back later and read this). In my main for receive I allocate an ArrayBuffer(250) once, and share it across many reads. When LoRa get 10 bytes, it puts 10 bytes into the ArrayBuffer and returns the size (10). Buffer size is still 250, but the last 240 bytes are all zeros. Using String.fromArrayBuffer works because the rest of the buffer is nulls.
Later we might receive 5 bytes. Those transfer into the first five bytes of the ArrayBuffer and 5 is returned - all correct. But the buffer still have the prior bytes, so the first 10 bytes remain with values before the null is found.
When we use String.fromArrayBuffer, there is no other length to use so we get all 10 bytes in the resulting string. I don't believe we can say String.fromArrayBuffer(buffer, size), as they would provide the size hint instead of depending on null/length.
Therefore this issue is about using String.fromArrayBuffer and not so much in LoRa itself, though we might be able to help avoid issues if we append a null after receiving a packet (assuming space available in the shared buffer). Of note, if dynamically allocated buffers are used for each read this would not occur.
Thanks, Peter, I understand now. Just to give me a sense on how to proceed knowing this, how important has it been, historically, to stay up to date w/Moddable releases? Do you report on critical vs. functional changes (or is that just the release notes and making making our own decision on the criticality of the changes)? If you know, how many critical (security, reliability) required updates have occurred in the last couple years?
Also - is the Moddable SDK linked to XS for releases, meaning can I take the SDK updates/fixes and not XS? I would guess most security / reliability issues are in the SDK (but perhaps I'm wrong on that?)
read() (if buffer has extra room). Solved the problem as expected. If a different solution makes better sense, glad to change. Gist here