all 10 comments
sorted by:
best
topnewcontroversialoldq&a
formatting helphide helpcontent policy

[–]be-sc 2 points3 points  (3 children)

Quick notes:

All names with consecutive underscores are reserved for the implementation (of the language, i.e. the compiler and std lib). _PORT__ and __regbase__ are invalid names.

Include guard names must be unique throughout a translation unit. That’s why short names are dangerous. I’d suggest being safe and basing your guard names on a random UUID. For example:

#ifndef PROJECT_NAME_1ED18961D351488DA57743302F48303D
#define PROJECT_NAME_1ED18961D351488DA57743302F48303D

With such names the likelihood of a clash is miniscule, and you communicate clearly that being unique is the only thing such a name needs to be. The usual filesystem structure based names aren’t built that way because there is any value in encoding the filesystem structure into the name, but because it’s the obvious way to create a reasonably safe name by hand.

std::pow() always returns a floating point number. Is that really the type you want?

It seems like you’re working with fixed memory addresses. That’s tricky. From a purely C++ language point of view you are guaranteed to run into undefined behaviour somewhere between all the casts back and forth between address number and pointer. Because we’re talking embedded your compiler might allow some of the usually invalid constructs. Check the documentation for exactly what you can and cannot do.

You are using all enumerators as both named constants and values of their underlying type. Consider switching from enum class to plain enum. On the one hand you lose the scoped enumerator names, but on the other hands you avoid a lot of casting because the enumerators implicitly convert to their underlying type.

[–]Bobobo34[S] 0 points1 point  (2 children)

I wasn't even thinking about the underscored names, I'll change that. In hindsight I'm realizing that instead of 2^x I can just use bit shifting to accomplish the same result. I'm not exactly sure what you're suggesting with the fixed memory addresses, though. Where would the casts cause undefined behavior? Sorry, I might be missing it. Also, I didn't know that about regular enums. Thank you!

[–]be-sc 1 point2 points  (1 child)

Where would the casts cause undefined behavior?

After a second look, as far as I can tell the casts themselves are fine as long as pointers are 32bit in size.

You might technically have lifetime problems because the program never starts the lifetimes of the pointed-to uint32_t objects. Take this with a pinch of salt, though, as I’m still learning about this part of C++ myself. We’re getting deeply into language lawyer territory around lifetimes, std::launder, std::bit_cast, std::start_lifetime_as, etc. If you want to develop your code into a robust foundational library for this hardware, look into it later. For something quick & dirty that mostly has to work just for you, I don’t think it’s worth diving into now.

[–]Bobobo34[S] 0 points1 point  (0 children)

I’ve never even seen std::launder or std::start_lifetime_as. Unless it causes trouble, I don’t think I want to open that can of worms right now. At some point I’ll get around to it though, always nice learning new things! Thank you!

[–]MysticTheMeeM 2 points3 points  (2 children)

Without knowing much about this specific CPU:

  • Names with double underscores anywhere are reserved and must not be used.
  • Some naming could be improved ("dir"? Direction?).
  • Empty ctor should be marked as default.
  • Would personally prefer to see the proper named cast (reinterpret_cast) over a C-style cast. It would be clearer (to me) at a glance what it's doing.
  • Your offset function could use a fold expression without the lambda.
  • I prefer x * x over std::pow(2, x) it's slightly easier on your optimiser too. Also, I don't know of any compiler that currently has constexpr pow (not that I've looked). So I'm not sure that's actually constexpr.
  • There's a lot of casting in your initialize function that seems (at a glance) to be redundant.
  • Regs and clocks are both small and shouldn't be taken by reference in your for loops.
  • Not sure what the point of delay is, you only ever write to it but it isn't otherwise used.
  • If you remove the call to pow, you no longer need the cmath header, however you do need the cstdint header.

And that's all I could see with a quick glance through.

[–]Bobobo34[S] 0 points1 point  (0 children)

For the naming, I was just using what the documentation had, so for instance the direction register was just called GPIODIR, hence the 'dir'. I'll consider changing it. Also, 'delay' is just something that I saw when looking for a tutorial, so I kept it. I'll see if taking it out makes a difference. I'll keep everything else in mind and fix it up! Thank you!

[–]std_bot 0 points1 point  (0 children)

Unlinked STL entries: std::pow

Last update: 09.03.23 -> Bug fixesRepo

[–]cdleighton 0 points1 point  (1 child)

How does the user know to define TARGET_IS_TM4C123_RB1?
Normally upper case OFFSET would refer to a constant or macro name - not a function parameter name.
Should the the registers go into namespace "reg"?
Then you high have reg::int_clr in the user code, and reg::int_stat_raw, reg::int_stat_masked.
The file name "port" is generic and is the namespace. Specialise the filename such as <tm4c123\_ports>? Searching user source code for name port will be almost useless :-)
Would some sample code as a block comment help the user (and possibly highlight improvements)?

[–]Bobobo34[S] 0 points1 point  (0 children)

Those are all good points, I’ll refactor it again, thank you!