search

lancaster-university / microbit-dal

http://lancaster-university.github.io/microbit-docs
Other
254 stars 130 forks source link

filesystem fix, radio packet size config #421

Closed xenomote closed 11 months ago

xenomote commented 5 years ago
martinwork commented 5 years ago

This would have been better as several separate PRs.

MICROBIT_RADIO_MAX_PACKET_SIZE should not be changed. It's not the radio maximum packet size. It's the maximum packet size for clients of the DAL. It's the size of the payload array in the FrameBuffer structure. Making it bigger will eat a chunk of RAM. While looking at this, I noticed that the range check on len in MicroBitRadioDatagram::send looks wrong.

MICROBIT_RADIO_DEFAULT_FREQUENCY should not be defined in MicroBitRadio.h. This became a config here https://github.com/lancaster-university/microbit-dal/commit/7aedfab59ac74cf74d8ec906f3aab9f5bcb1e6af.

I don't think a const operator[] needs to return a reference, but I could be wrong!

If PacketBuffer::getBytes is to be const, the pointer it returns should also be const. Could it have been created non-const on purpose, instead of creating const and non-const versions?

In ManagedString::ManagedString, checking length against strlen seems reasonable.

xenomote commented 5 years ago

I understand it should have been several PRs but this is part of university project work with the microbit so the changes were made as I went along. Hence the default frequency is an accident, I'll remove that.

From what I could understand of the Nordic documentation and the existing code, it seemed that the MicroBitRadioDatagram info sits in the PAYLOAD section of the radio packet described in the Nordic docs, with the other fields S0, LENGTH and S1 unused it seemed that PAYLOAD could occupy the full 254 bytes. I may be wrong, but regardless, the original 32 bytes for payload seems needlessly limiting.

I removed the strlen check because it seemed redundant when the programmer provided length is available. Of course this puts the onus on the programmer to provide the correct length, but this doesnt seem unreasonable. Alternatively the length parameter could be dropped and strlen used instead, but that would be less efficient. I may have the wrong intentions but I'd like to hear your thoughts.

the smattering of const's was born of me trying to subclass the packetbuffer for my own uses, and in certain cases the const and pass by reference was required to satisfy the compiler (I can't remember the specifics because it was a while ago) but they were all quite necessary. If you need more detail then I can spend some time retracing to work out the details.

I'm not intimately familiar with C++ so I may be making some naive mistakes, but most of these changes seem justified

martinwork commented 5 years ago

Maybe MICROBIT_RADIO_MAX_PACKET_SIZE could be a config option. You may be right about ManagedString. I am not sure what was intended.

xenomote commented 5 years ago

Alright, I'll look into making it a config option. Do you think there's anything else I've missed?

martinwork commented 5 years ago

Please note I can't merge PRs. I'm just trying to be helpful! I think I have mentioned everything I spotted.

Do you actually need larger radio payload? I would make a separate PR, which would make it more likely to get merged.

Did the strlen check in ManagedString cause a problem? It has always been there, apparently because ManagedString was not intended to handle strings with zero characters in them.

xenomote commented 5 years ago

Oh okay, I thought you were a member of the project :'D

The payload size is fairly important, as I said the 32 byte limit is prohibitively small for what my project (and I imagine many others') needs to achieve. I suppose I should split the PR but we'll see what the reviewers say.

The strlen check wasnt causing a problem as such, and I suppose I might have had the wrong idea about the intent. I guess the manual length might be used for limiting the input to a fixed maximum length, however in that case it doesn't make sense to discard the string entirely if it is too short, so I'll change it again to reflect this.

Thank you for your suggestions regardless

martinwork commented 5 years ago

In 6 months time, an entry in the commit log saying "bugfixes and tweaks to filesystem" will not be as helpful as a sequence of focused fixes for specific issues, especially if the changes under that heading do not all relate to the file system. Making a PR as simple as possible makes it easy for a reviewer to check and accept.

pelikhan commented 5 years ago

Add the config code to be able to change the radio packet size via a yotta config . Do not change defaults as they might break existing scripts.

Get Outlook for iOShttps://aka.ms/o0ukef

From: Martin Williams notifications@github.com Sent: Sunday, February 3, 2019 1:35:14 PM To: lancaster-university/microbit-dal Cc: Peli de Halleux; Comment Subject: Re: [lancaster-university/microbit-dal] bugfixing and tweaks to filesystem (#421)

In 6 months time, an entry in the commit log saying "bugfixes and tweaks to filesystem" will not be as helpful as a sequence of focused fixes for specific issues, especially if the changes under that heading do not all relate to the file system. Making a PR as simple as possible makes it easy for a reviewer to check and accept.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flancaster-university%2Fmicrobit-dal%2Fpull%2F421%23issuecomment-460090838&data=02%7C01%7Cjhalleux%40microsoft.com%7C1db409973a904705e64508d68a1f7bf6%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636848265158961193&sdata=Ifb2SyP1nj5ZXXlVICily%2F32w5uj0JqGZmeCh1A4JWc%3D&reserved=0, or mute the threadhttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAD-4KfuzNwI_viPVnbRYxlVpef66Zc7zks5vJ1YSgaJpZM4ac0dX&data=02%7C01%7Cjhalleux%40microsoft.com%7C1db409973a904705e64508d68a1f7bf6%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636848265158961193&sdata=JvR0%2FZHu4Ga6G8gNldKeMPzWEJ0DHdRjNJmTFcpuQfA%3D&reserved=0.