cederom
Change all types to use stdint types
There's a lot of use of int's in this lib, in some case they should be unsigned, in others we only need 8 bits etc.... Additionally since sizeof(int) is defined per compiler, it's recommended to use the more explicit stdint.h types.
This would be a lot of work, but I think it would help code readability.
Yup, the assumpion here we have at least 32-bit int, oherwise char is used for 8-bits. I was considering once to use 8-bits only but that seems lots of additional work... on the other hand could allow working on cheap 8-bit MCUs..
I might have a crack at making the change at some point. Just wanted to check that you agreed it'd be a good idea first.
On 23 June 2017 at 05:20, Tomasz CEDRO [email protected] wrote:
Yup, the assumpion here we have at least 32-bit int, oherwise char is used for 8-bits. I was considering once to use 8-bits only but that seems lots of additional work... on the other hand could allow working on cheap 8-bit MCUs..
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/cederom/LibSWD/issues/13#issuecomment-310616989, or mute the thread https://github.com/notifications/unsubscribe-auth/AE51OmJqSw4IAlFWcErDJ1neCRxB4f4Oks5sG4NYgaJpZM4OC3w7 .
Would that imply any portability/platform/compiler issues/restrictions/obligations? I considered int to be most generic..
Not really a priority right now, but of you have time and energy to work on that feel free Andy :-)
While int is more generic and it is possible that stdint.h wouldn't be provided for some toolchains / with some compile args, it does have the several benefits:
- uint8_t is always an unsigned 8 bit value, on every platform, similarly uint32_t is always an unsigned 32 bit value. Whereas unsigned int is normally 32 bits, but could be 16 or even 8 bits, or possibly 64 bits depending on toolchain.
- readability. When you have a type of uint8_t, it's pretty obvious that you only expect this value to be between 0 and 255.
This link is an interesting read: https://stackoverflow.com/questions/9834747/reasons-to-use-or-not-stdint
Basically the only down side is efficiency, however I don't think that's particularly critical in this lib, where speed will largely be how fast you send your comms signals.
On 23 June 2017 at 17:11, Tomasz CEDRO [email protected] wrote:
Would that imply any portability/platform/compiler issues/restrictions/obligations? I considered int to be most generic..
Not really a priority right now, but of you have time and energy to work on that feel free Andy :-)
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/cederom/LibSWD/issues/13#issuecomment-310774109, or mute the thread https://github.com/notifications/unsubscribe-auth/AE51OobKRWAooArooQTG9aegO7r93i45ks5sHCoZgaJpZM4OC3w7 .
Andy, thanks for reference :-) Why efficiency could be impacted? What about the memory footprint?
We have Thomas from Czech that already ported LibSWD to a microcontroller (32bit ARM) and has noted problems with memory management.. maybe he could also compare the possible efficiency changes..
Hello Andy, Cederom, Let me say few words about using stdint.h. In embedded system this is must have and it is also absolute base stone requirements. As they discussed in stackoverflow, it is much easier and safer to port such code which uses stdint.h. If such compiler, which do not support stdint.h exist, than it is very easy for developer to create his own definitions. I am using the library on STM32F205 CPU, and it is running well, but integration and calling functions has lot of redefinitions like int to u32, char to u8 and so on. In conlusion I would say that I recommend to use stdint.h absolutely everywhere, where it is possible. Using pure int just in case, where you wanna grow with CPU bus width in future usage.
I have found some issues with libswd_error which is holding his history in error cmdq pointer, which is absolutely not handled and not freed anywhere. This can cause runtime memory depletion. Also this error part does not count whit my memory limitation implement. Need to work out a clean solution.
At this time I have tested flash readout from STM32F030 and STM32F207 with success. On my todo list is an automatic CPU recognition and then try flash a new FW into target.
Firstly the efficiency comment:
If you have a native 16 bit system and your code has a loop between 0 and 3725 (for example), you could use uint16_t because it fits in there, but maybe you're not thinking about the limits that much (or to be safe for future expansions) and so use a uint32_t. The compile now has to insert code to deal with a 32 bit number on a 16 bit system. so i++ translates to:
lower ++; if (lower == 0) { upper ++; }
where lower and upper are 16 bit numbers. Obviously this is less efficient than i++
However it depends on use case, if you genuinely need a 32 bit counter, then obviously you need this code regardless. If however you will never count higher than 65534, then a uint16_t would work. If you never count higher than 32, a uint8_t would work. So one option is just to use a normal unsigned int at this point, where the compiler will give you the best option (and hopefully warn you if that's not good enough).
On the other hand, someone in the stackoverflow thread talks about the opposite effect. A 32 bit system where you use uint16_t. I'm not entirely sure how this is less efficient. One idea example might be (kind of making this up, so don't count it as proof).
uint16_t myArr[20]; ... for (int i = 0; i < 20; i++) { myArr[i]++; }
now if myArr is packed (two elements per 32 bits) and the architecture doesn't have separate 16 bit vs 32 bit operations (x86 has AL, AH, AX, EAX, which give you 8, 16 and 32 bit ops, not sure if by default arm has similar, I don't think it does).
so now you have to increment the lower 16 bits, while anding out the top bits, storing it in the lower 16 bits again and then repeat for the top bits.
Normally compilers deal with this sort of thing by not packing things, and storing each element in a word, but I'm not sure how this works in arrays.
As mentioned in the thread there is a uint_fast16_t or similar, I've never used that and only heard of it yesterday. This lets the compiler make it bigger if it's more efficient, so in the above case, my array would probably be of 32 bit ints. Obviously this is then a trade off between memory usage and efficiency.
I would argue that your library blocks on sending signals to the outside world, (I'm not sure on the max supported SWD speed, but even if we assume it's 10Mbits), that's still going to be your bottle neck apposed to the compiler throwing in some extra instructions
I think all / most toolchains will support stdint, but I have seen options to not use stdlibs or std headers, which I think would break this. However as Tomaz says, adding your own definitions isn't too hard, you just need to be careful and know exactly what size is each type, and then do typedef uint8_t unsigned char; etc...
Other Tomaz: on the cmdq note, viaExplore has submitted a pull request that involves a limit to how many items can be stored in cmdq. I haven't looked at it, but that should hopefully do what you want. I fixed this in my user code, by adding the following code in a bunch of places:
_gLibswdctx->cmdq = libswd_cmdq_find_head(_gLibswdctx->cmdq); libswd_cmdq_free_tail(_gLibswdctx->cmdq);
Not perfect, but does work. If viaExplore's patch works I'll switch to that as a much better solution.
Andy
On 24 June 2017 at 06:30, Tomáš Kamenický [email protected] wrote:
Hello Andy, Cederom, Let me say few words about using stdint.h. In embedded system this is must have and it is also absolute base stone requirements. As they discussed in stackoverflow, it is much easier and safer to port such code which uses stdint.h. If such compiler, which do not support stdint.h exist, than it is very easy for developer to create his own definitions. I am using the library on STM32F205 CPU, and it is running well, but integration and calling functions has lot of redefinitions like int to u32, char to u8 and so on. In conlusion I would say that I recommend to use stdint.h absolutely everywhere, where it is possible. Using pure int just in case, where you wanna grow with CPU bus width in future usage.
I have found some issues with libswd_error which is holding his history in error cmdq pointer, which is absolutely not handled and not freed anywhere. This can cause runtime memory depletion. Also this error part does not count whit my memory limitation implement. Need to work out a clean solution.
At this time I have tested flash readout from STM32F030 and STM32F207 with success. On my todo list is an automatic CPU recognition and then try flash a new FW into target.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/cederom/LibSWD/issues/13#issuecomment-310830594, or mute the thread https://github.com/notifications/unsubscribe-auth/AE51OsqXq0PIRg5hpVjHYOhCGNX33f4Cks5sHOVEgaJpZM4OC3w7 .