memcpy() and volatile

[Nominal Animal]

Refactoring is what you do later on when you discover an ACTUAL requirement that can not be easily implemented in the current design.

Or when the current design maintenance cost is higher than the cost of replacing it with a simpler, easier to maintain design.

You refactor the code -- that is, change its structure without changing the functionality AT ALL -- until it is in a form where the now known to be needed functionality is easy to implement.

Without changing the used functionality; unused features and parts of the design are (or should be) removed.


Refactoring things to minimize code complexity and cost of code maintenance is a very important part of my work flow.

Story time, feel free to skip/ignore:

My own design approach differs from the most common ones, in that I like to test important features in modular test programs/firmwares before using them in a design.

For example, let's say you have an embedded design where for some reason, you need many lightweight timeouts that often/typically get cancelled before they elapse.  (This is very common in systems programming, especially in any kind of internet or web service, and one I have lots of experience with; that's why I'm using it as an example here.)  My preferred solution is a combination of a binary min-heap in a fixed-size array, with each entry containing the timeout time (when it elapses/triggers) and the timeout index/id.  In parallel, there is an array with the same number of events, with each entry containing the current min-heap index, plus any associated information needed for the implementation (like elapsed flag, or whatever happens when the timeout elapses).

I start by creating a basic min-heap implementation, with the entries using appropriate types for the target processor and maximum number of concurrent events.  (I typically use an unsigned integer type for the timestamp.  If the maximum allowed timeout (duration) is N-bit, then N+2 bits suffices for the timestamp, giving me absolute reliability according to tests I've done before.  Others can do it in N+1 bits, but often end up having off-by-one errors near the maximum duration timeouts.  With non-power of two durations, three times the maximum timeout duration suffices for timestamp range.)
The test implementation is run on a fully-hosted system (Linux), with inputs supplied from files, so I can easily construct test cases, including the worst cases I can imagine – those being the most important to test for in my opinion.
When the basic min-heap works as I need it to, I save it as a separate unit test, but continue with adding the identifier array for canceling timeouts and triggering timeout events.  If there are different event mechanisms, I test those separately, without the min-heap at all.

When that sub-system has been tested and works, I add my comments on my current ideas on how it will be used in the final firmware/application, but do not integrate it yet.  I make sure I have all the important modules working in isolation first.

When I have all the modules tested, I start the integration work.  While one could just add all the modules at once, I prefer to do it one module at a time, because that way I minimize the area where bugs and errors and issues can arise.  After adding a module, I do full testing, including the new tests for the added module, of course.

If the firmware/application is complex enough, I often realize that two (or three) modules could/should be merged, because that would yield better, easier to maintain code.  (If it adds to the code complexity, I consider merging having negative value, because the code I write tends to get used for a long time, so cost of maintenance is as important as cost of creation to me.)  I end up refactoring the modules in a separate unit test/module; usually not within the actual firmware/application directly.  (I tend to have learned more about exactly what features matter, so I do tend to rewrite the tests, instead of just reusing the existing ones from the modules; but test coverage should always expand, and not shrink, here.)  Reintegrating into the firmware/application is then done just like for any other module.

This leads to things like having to carefully document the internal interfaces to each subsystem.  This makes refactoring easier, because from the interfaces actually used in the firmware/application, you can tell if parts of the "modules" are unused, and can be discarded or refactored.  It also explains why I find modular programming and software minimalism (and their combination, the Unix philosophy) so useful: as a proven historical track record, this approach produces good results.

This serves me well, but makes me somewhat less productive in terms of 'lines of code of new features per day'.  My bug rate is much lower than typical, and I tend to keep core documentation up to date because I rely on it myself, so I consider this well worth it; but many, especially manager types worrying about the production costs, often disagree.  (I still do struggle with writing truly useful code comments, though.  Should have learned to do that from the get go, instead of later on!)  So, I do not recommend adopting this for those who do paid commercial work, because it can reduce their attractiveness in employers eyes.  I am not limited by that, as I only work for free or for myself, currently.

[DiTBho]

@Nominal Animal
I did/do the same, so I totally understand you!

let's say that among the various approaches (which are all correct), even this is a good way to proceed, but it's special because at least it garanties that bad surprises are avoided, and things move forward in a methodological way, which is excellent especially if you work as a team!

as a side effect then, it favors documentation, precisely because you focus on one module at a time, and documentation is necessarily useful when stitching everything together.

[DiTBho]

The simple fact is that if you have onboard CAN bus on your micro controller you effectively have a comms co-processor.

yeah, this is an extra-bonus 

[Simon]

The simple fact is that if you have onboard CAN bus on your micro controller you effectively have a comms co-processor.

yeah, this is an extra-bonus 

In my case a double bonus as there are two independent CAN bus controllers and as I am stuck with the design of the HMI until I replace it I can run it on one bus and the motor drivers on another bus so I do not risk upsetting anything.

The other thing I forgot to list that the CPU would have to do with a UART based bus is filter every message. Every message must be received and filtered by the program. With dedicated CAN bus controllers not only is the CRC stuff done by the controller but it also worry's about whether or not you wanted that data and it only informs the main program about messages that the main program actually wants. on a network of many devices using most of the time you spend on comms rejecting messages that are not wanted would seriously overshadow the time spent on the messages that the program does want if you use a UART based bus.

[Mechatrommer]

Refactoring is what you do later on when you discover an ACTUAL requirement that can not be easily implemented in the current design.
You refactor the code -- that is, change its structure without changing the functionality AT ALL -- until it is in a form where the now known to be needed functionality is easy to implement.

i dont know you people in professional industry, but me with my hobby when i start a project, i usually just code whats needed, but when it grow a little bit larger i figured my functions keep duplicating the same thing, or too many little little separated units/functions/modules, thats where i quickly refactor separate or combine. and since some experience in belt, when there is a function needed and i know from the past that function is commonly called, then i will do it in separate function in first place. refactor when the code is already huge is quite a task since you need to keep all functions in memory to know what to stripped off and what to add or combine etc. sometime it took 2 or 3 times refactoring during code development, but thats just me.

[Siwastaja]

The motor drivers use CAN Open and that is the end of that.

Then don't implement fully generic CANOpen. You don't need other profiles than the motor drivers. Just support the message types needed by the motor drivers.

[Simon]

The motor drivers use CAN Open and that is the end of that.

Then don't implement fully generic CANOpen. You don't need other profiles than the motor drivers. Just support the message types needed by the motor drivers.

Other profiles? do you know how many there are? tons I think. Of course I have only implemented what is needed by the motor drivers. They need the CAN Open comms to work as expected. All that the profile is about is the list of things stored at various index locations and any other functionality like foc DS402 the power up.

But CAN Open communication is no simple matter.

With my massive 16kB of RAM I can only setup the objects that i will actually use, if I were running it on computer like a raspberry pi I'd have an easier time just allocating the entire dictionary space and just using using it or not.

[SiliconWizard]

Just a thought here regarding your last sentence - there's a very wide range of MCUs out there in terms of RAM size, between 16KB and the several GB a SBC supports.
Surely you must have had a reason for choosing this limited MCU for this application, maybe the choice isn't the wisest, I don't know. Even a RP2040 which is less than $1 has 264KB of RAM. Anyway. Not that what you wanted to achieve all along is impossible with 16KB - looks like a few people have given tips and approaches. But if that really makes your development this uncomfortable, selecting a different MCU may have been a relatively cheap approach.

[Simon]

Again it was politics. I need to develop a HMI and master ECU around a single board computer but that would have been a step too far, as it is it has taken 2 months just to get to understand what CAN Open requires and I still need this micro controller with the CAN bus set up on it. So I only rocked the boat as much as I needed to.

No micro controller will have the RAM for a full easy CAN Open master. The definition of the CAN Open object dictionary is that each object is addressed by an index of 0-2^16 and a sub index that can go up to 255.

If you take this to the extreme this 2^24 of combinations is 16million. If I were to use the system I came up with of assuming int32 will contain any object that is 64MB per slave device. So to replicate the slaves object dictionary in the master I could take the easy way of an array of:

int32 objects[65535][255] ;

or better for the entire network:

int32 objects[n_nodes][65535][255] ;

that is quickly a huge amount of RAM. Now of course I will never need to actually map the full 2^16 indexes or 2^8 sub indexes and I could have 3 arrays, one for each of the sections of object dictionary: communication settings, manufacturer specific section and profile section with only the range that I actually use although this is making the code less universal.

So the solution here was instead to simply have each object in a structure where the index and sub index is stored as well and these are in an array. I then had to create an index of the object names as an enum list so that I can search through my limited dictionary.

No microcontroller will have the MB of RAM needed to create the dictionary the easy way. I am also in a condition where ironically my lower power CPU has more work to do to address an object as there is no natural way to call the index and sub index, but it is often not a problem. On something with enough RAM I could simply use the index and sub index. So finding the object that an SDO message addresses is very fast as I would simply use the index and sub index in the message to address the array rather than have to search through the indexes slowly with a slower CPU. A SBC with enough RAM would have a much easier job as it can simply address the dictionary instead of search.

So any micro that does not have many MB of RAM will have to go through what I have here. The SAMC is simply what I know and have code for, there is a 32kB version but there were not available when we bought stock, there are other compatible MCU's higher up the product range but it's still a case of kB versus 10's of MB which is the difference between a micro controller and a computer that can run an OS like linux.

[gf]

How many entries does the dictionary actually contain? Tens, hundreds, thousands,...?
Since you say that an array implementation would be far too sparse and take up too much memory, have you considered a hash table or tree implementation for the dictionary?

[Berni]

You need to implement a dictionary structure rather than a simple array. You can get those in C++ and all the modern ARM compilers can take C++ code.

Also if you do want lots of RAM in a MCU, that's no problem. Lots of larger MCUs support an external memory bus that can run SRAM or SDRAM memory. The amount they can actually address varies a lot but should be somewhere in the range of 8 to 256 MB

Still none the less this sounds like a bad excuse for needing that much RAM

[brucehoult]

You refactor the code -- that is, change its structure without changing the functionality AT ALL -- until it is in a form where the now known to be needed functionality is easy to implement.

Without changing the used functionality; unused features and parts of the design are (or should be) removed.

STRONGLY disagree!

Adding or removing functionality of any kind is not a legitimate part of refactoring

Removing unused functionality may certainly be a good thing, at times, but it's not refactoring. It's before and/or after refactoring.

i.e. refactoring may make it easier to remove unused functionality, and removing unused functionality may enable a refactoring simplification.

But they are separate steps and separate commits.

[Nominal Animal]

You refactor the code -- that is, change its structure without changing the functionality AT ALL -- until it is in a form where the now known to be needed functionality is easy to implement.

Without changing the used functionality; unused features and parts of the design are (or should be) removed.

STRONGLY disagree!

I specifically refer to omitting unused/dead code, not adding new functionality.  To me, "refactoring" = "replacing an existing chunk of code with its functional equivalent", i.e. not changing its externally observable behaviour.

I take it you prefer the stricter definition, where that is called "rewriting" or "replacing", and "refactoring" only refers to functionally identical code?

I keep the modules so small I get no benefit from splitting the functionality removal and the rewriting into separate steps.  Now, I do admit, I sometimes even modify the function call interfaces at the same time –– which is definitely beyond refactoring in my opinion –– if the modules are small enough and I've documented the interfaces well enough.

But they are separate steps and separate commits.

Separate commits definitely makes sense, so that at each commit the source code is in as valid state.

I don't often use source control for my own stuff, as I do better documentation when working on module-sized chunks (compared to changelogs).

Writing descriptive comments, using source control, writing descriptive changelogs, and then separating dead code removal from functional rewrites makes a lot of sense, and is indeed what I recommend others do.  What I do is definitely not the optimal or recommended way.  There is always room for improvement!

[Mechatrommer]

i think refactoring is including cleaning up once used but now unused code, but not adding extra functionalities, the purpose is more readable/maintainable to what is "already existed" https://refactoring.guru/refactoring

[DiTBho]

I take it you prefer the stricter definition, where that is called "rewriting" or "replacing", and "refactoring" only refers to functionally identical code?

"Doors" terminology, applied to a project under DO178B:

• "refactoring"(modules) -> modules must have exactly the same interfaces and global variables
• "sweep"(modules) -> dead-code (unused { functions, global variables, header structures and const}, ...) removed

two steps, two documents, to be sent to the QA guys

for my personal projects I do both things in parallel, or first I do sweep, and then refactor.
Probably it's not correct, but I do this for the simple fact that I have less work to do,
and that I do these things in my spare time.

(1) it's like Git, but commercial, used in Avionics.

[Nominal Animal]

i think refactoring is including cleaning up once used but now unused code, but not adding extra functionalities, the purpose is more readable/maintainable to what is "already existed" https://refactoring.guru/refactoring

The exact definitions of the terms are not that important, as long as everybody understands what is meant.

brucehoult's core point that when using version management, you should first remove the unused code as a separate commit, before replacing the code in another commit, is quite valid: that way, each commit yields a functional, compilable, testable state.

This is particularly important/useful when hunting for complex or rare interaction bugs that require commit bisection (binary search for the commit that changes the behaviour wrt. the bug or issue): it minimizes the changeset per commit, while keeping the tree in compilable, testable state in each commit.

[Simon]

How many entries does the dictionary actually contain? Tens, hundreds, thousands,...?
Since you say that an array implementation would be far too sparse and take up too much memory, have you considered a hash table or tree implementation for the dictionary?

I don't quite understand what those are but I suspect what I have implemented is close to one of them. I have a structure type that stores the index, subindex type and values. I have set up an array of these structures. I have a list (enum) of the names that I use to index the array in English.

[Simon]

You need to implement a dictionary structure rather than a simple array. You can get those in C++ and all the modern ARM compilers can take C++ code.

Also if you do want lots of RAM in a MCU, that's no problem. Lots of larger MCUs support an external memory bus that can run SRAM or SDRAM memory. The amount they can actually address varies a lot but should be somewhere in the range of 8 to 256 MB

Still none the less this sounds like a bad excuse for needing that much RAM

Ultimately if I am seriously implementing CAN Open and I want to scale I need something more suitable. The cost of a SBC is trivial in my line of work and I don't want to keep changing implementation. I also need to run a screen anyway and will be figuring out programming under linux soon.

As for actual RAM needs my current method will probably also work quite well without much overhead. I had misunderstood the SDO protocol at the start, now that I know better I don't need to keep track of the index I sent and the the one that I got back, I need to remember the last slot I sent (the array index) and when I get a response I check to see if the received index corresponds to that of the one I saved the index of when I sent, no searching required. I totally over egged it.

[westfw]

Refactoring is what you do later on when you discover an ACTUAL requirement that can not be easily implemented in the current design.

I guess if you believe that a complete specification is the hard part of software development, then having a working "product" and just a "small" set of new requirements is a lot closer to have that "complete specification" than you usually get.  (and probably much better than you had when the previous version was written.)

[Simon]

This is what I have had to put up with. I had to have an argument and stick to my guns over moving to CAN Open control as many saw a working system on analog inputs.